feat: layered opt-in permission system (P0–P2.2) — rules, approval, audit, guardrail prompt

[ncrispino]

Summary

Builds out an opt-in, layered permission system for MassGen: declarative allow/ask/deny rules + risk-tiered approval, an interactive approval modal, headless/automation policies, an append-only audit ledger, per-agent role scoping, and a channel-based guardrail system prompt. Fully presence-gated — a config with no permissions: block is 100% unchanged.

This PR is the full feat/permissions-p2-audit branch (P0 → P2.2). The later commits (this work session) close gaps the earlier layers had advertised but not wired, add the guardrail prompt, and make denied tool calls first-class in the TUI/WebUI.

What's included

Engine & approval (P0–P1)

• Hardline catastrophic-command blocklist (safety floor) + composite permission engine (declarative action(target) rules → risk classifier → allow/ask).
• ask → approval round-trip in the base_with_custom_tool_and_mcp chokepoint; interactive approval modal (allow once/session/always · reject), automation policy provider (risk-based/deny-all/allow-all), and file provider for headless/remote.
• Per-agent role presets (e.g. read-only) merged deny-wins; read-only also empties the SRT writable set (OS backstop).

P2.1 / P2.2 (this session)

• ApprovalLedger append-only JSONL audit trail (now actually wired) + ApprovalBudget runaway-loop guard (opt-in via max_consecutive_auto).
• always-grant persistence — full write + read-back loop to settings.local.json (makes the modal's "Always" real).
• Backend parity guard — native backends (claude_code/codex) lack the chokepoint, so the engine is reported INACTIVE with a loud warning instead of silently inert.
• Channel-based guardrail system prompt (when permissions active): follow the guardrails, don't circumvent denials, surface-and-ask; ask is explicitly the sanctioned path (no token — authority is established by channel, not a leakable secret).
• Denied tool calls are now first-class FAILED tool events — the deny path emits tool_start (with the attempted command) + tool_complete(is_error=True, status="denied"), and the status line shows the command (Denied ($ curl …): …).

Honest limitations (documented, not hidden)

The regex risk-classifier is a porous denylist and the guardrail prompt is best-effort alignment, not enforcement. Live gemini-3-flash-preview runs showed the model evading egress via \c\u\r\l and python urllib, and the prompt's effect is inconsistent (obeyed in one run, bypassed in another). The OS sandbox (SRT) is the load-bearing egress control — tracked as follow-up.

Test plan

• Unit/integration (TDD): rules algebra & deny-wins-across-scopes, risk tiers, hardline, coordinator + ledger + budget, file/policy/callback providers, persistence roundtrip, parity guard, guardrail-prompt gating/content, denied-tool-call event emission. All green.
• Config validation: 287 configs valid; pre-commit clean.
• Live automation smoke (gemini-3-flash-preview): verified all three chokepoint branches end-to-end (allow / deny-rule / ask→policy-deny + ledger), the guardrail policy present in the real system message, and denied calls emitting real tool_start/tool_complete(error) events with the command.
• Manual (cannot automate): interactive modal round-trip, cross-run "Always" persistence, SRT OS-enforcement — see follow-ups.

Demo config: massgen/configs/tools/permissions/permission_modal_interactive.yaml (interactive modal + automation deny paths).

Follow-ups

• WAN-33 — limitations & manual-test gaps (ledger scope, glob-metachar matching, manual modal/persistence verification).
• WAN-34 — OS-layer (SRT) egress enforcement; the load-bearing fix for the bypass evidence above.
• Full write-up: docs/dev_notes/permissions_p2_followups.md.

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• New Features

  • Added permission system enabling declarative tool-use controls with deny/ask/allow rules and risk-tiered approval workflows
  • Introduced interactive approval modal dialogs for human review of tool calls during execution
  • Added read-only agent role to restrict write operations and network access
  • Added permission guardrails section to system prompts enforcing policy compliance

• Documentation

  • Added permission systems research and design documentation
  • Added permissions implementation follow-up notes and acceptance criteria

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

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: ccb6b26f-e142-478e-98e1-e3370672f441

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

Walkthrough

This PR implements a comprehensive opt-in permission system for MassGen that enforces tool-call authorization through declarative rules, risk-tiered approvals, and human-in-the-loop decisions. Permission decisions flow through pre-tool hooks, a permission coordinator that manages caching and approvals, system-prompt guardrails, and optionally an interactive TUI modal for human approval. An SRT read-only mode provides OS-level enforcement for read-only agent roles.

Changes

Permission System Core

Layer / File(s)	Summary
Core Models and Activation massgen/permissions/models.py, massgen/permissions/activation.py, massgen/permissions/__init__.py	Enums for RiskTier, GrantScope, and AutomationDefault; dataclasses for AuthorizationObject (permission request) and ApprovalDecision (approval outcome); activation logic (is_permissions_enabled, guardrail_prompt_active) that determines when permissions apply and when system-prompt guardrails render.
Declarative Rules and Risk Classification massgen/permissions/rules.py, massgen/permissions/risk_classifier.py, massgen/permissions/hardline.py	Permission rules with deny-wins semantics (PermissionRuleSet with allow/ask/deny precedence); action/target classification for tools; RiskClassifier that tiers commands by shell patterns, network operations, and destructiveness; hardline blocklist for catastrophic operations (recursive deletes, disk wiping, fork bombs).
Approval Providers and Coordinator massgen/permissions/approval_provider.py, massgen/permissions/coordinator.py, massgen/permissions/session_cache.py, massgen/permissions/ledger.py	Three pluggable ApprovalProvider strategies (policy-based automation, file-based JSON handshake for headless approval, callback for TUI modal); PermissionCoordinator that caches session grants, enforces consecutive auto-approval budgets, persists always-grants, and records decisions to audit ledger.

Backend and Hook Integration

Layer / File(s)	Summary
Permission Hooks and Hook Installation massgen/permissions/hooks.py, massgen/orchestrator_collaborators/midstream_injection_hook_installer.py	HardlineBlocklistHook and PermissionEngineHook that evaluate rules and classify risk; hook installer that merges role/config/persisted rules, registers hooks, selects approval provider, and installs permission coordinator on backend during agent setup.
Backend Ask and Deny Handling massgen/backend/base_with_custom_tool_and_mcp.py	Backend stores permission coordinator; PreToolUse hook handling expanded to support ask decisions (awaits approval, yields result); denied tools emit human-readable error StreamChunk with blocked-command preview and first-class FAILED tool-call telemetry events.
SRT Read-Only Configuration massgen/backend/base.py, massgen/filesystem_manager/_filesystem_manager.py, massgen/filesystem_manager/_srt_manager.py, massgen/backend/_excluded_params.py	Derives command_line_srt_read_only from permissions role config; wires through FilesystemManager to SrtManager; SrtManager clears writable paths for execution profile while preserving fs_tools snapshot/temp capability; permissions param excluded from API forwarding.

System Prompt and Interactive UI

Layer / File(s)	Summary
Permission Guardrail System Prompt massgen/system_prompt_sections.py, massgen/system_message_builder.py	PermissionGuardrailSection defines binding policy (approval is normal, denials must not be circumvented, blocks surface through approval flow, policy authority in system prompt only); conditionally added to coordination prompt when guardrail_prompt_active gates it based on backend support.
Interactive Approval Modal and UI massgen/frontend/displays/textual/widgets/modals/tool_approval_modal.py, massgen/frontend/displays/textual_terminal_display.py, massgen/frontend/coordination_ui.py	ToolApprovalModal renders risk/tool/args with allow-once/session/always/reject actions; textual_terminal_display bridges modal to TUI thread; coordination_ui swaps policy provider to interactive CallbackApprovalProvider for approved agents when TUI is active.

Persistence and Configuration

Layer / File(s)	Summary
Persistence of Always-Grants massgen/permissions/persistence.py	Callbacks derive durable action(target) rules from always-granted authorizations; rules appended to settings.local.json with deduplication; load_persisted_rules reconstructs rule set from prior runs, enabling approval "memory" across sessions.
Example Configurations massgen/configs/tools/permissions/per_agent_roles.yaml, massgen/configs/tools/permissions/permission_engine.yaml, massgen/configs/tools/permissions/permission_modal_interactive.yaml	Demonstrate per-agent roles (researcher read-only, implementer read-write), declarative deny/ask/allow rules with deny-wins, risk-based automation default, audit ledger, and interactive modal demo.

Design Documentation and Tests

Layer / File(s)	Summary
Permission Systems Research and Follow-ups docs/dev_notes/permission_systems_research.md, docs/dev_notes/permissions_p2_followups.md	Research synthesizes permission patterns from 11 agent CLIs; proposes three-layer arch (authorization/approval/guardrails), risk-tiering, and ask-chokepoint wiring; follow-ups document P2.2 implementation gaps, known limitations, and acceptance criteria.
Comprehensive Tests massgen/tests/test_permissions_*.py, massgen/tests/test_approval_*.py, massgen/tests/test_permission_*.py, massgen/tests/test_srt_*.py, massgen/tests/test_tool_approval_modal.py	Unit and integration tests for models, rules, risk classification, approval providers, coordinator caching/budgeting/ledger, hook execution, backend ask/deny, system-prompt gating, interactive modal wiring, persistence round-trips, and SRT read-only filesystem integration.

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly Related PRs

• massgen/MassGen#769: Introduced the general hook framework and PreToolUse hook interception that this PR's permission hooks rely on for deny/ask/allow decision injection into tool execution.
• massgen/MassGen#1125: SRT sandboxing improvements that this PR extends with read-only mode configuration and filesystem writable-set clearing for permission-enforced roles.

Suggested Reviewers

• Henry-811
• a5507203

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/permissions-p2-audit

[coderabbitai]

Actionable comments posted: 14

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)

massgen/filesystem_manager/_srt_manager.py (1)

217-226: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

read_only is skipped for the agent-facing fs_tools profile.

FilesystemManager.get_mcp_filesystem_config() and get_workspace_tools_mcp_config() both SRT-wrap the filesystem/workspace MCP servers with profile="fs_tools". Because build_settings() only empties writable in the non-fs_tools branch, a read-only role still gets OS-level write access through write_file / edit_file and workspace file-operation tools. That defeats the SRT backstop for read-only roles. Split a framework-only writable profile from the agent-facing file-tools profile, or apply read_only to fs_tools as well.

🤖 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 `@massgen/filesystem_manager/_srt_manager.py` around lines 217 - 226, The
FS_TOOLS_PROFILE branch currently grants write paths regardless of
self.read_only, letting agent-facing fs_tools gain OS-level write access; update
the logic in the function that builds settings (the block handling
FS_TOOLS_PROFILE / self.read_only — referenced by FS_TOOLS_PROFILE, writable,
self.fs_tools_extra_writable, managed) so that read_only is honored for fs_tools
too: either check self.read_only before adding any writable paths or inside the
FS_TOOLS_PROFILE branch set writable = [] when self.read_only is true (or
implement a separate framework-only writable list vs agent-facing fs_tools list
and only add framework-only entries when not self.read_only); ensure this fix
covers both callers mentioned (get_mcp_filesystem_config and
get_workspace_tools_mcp_config) which wrap servers with profile="fs_tools".

🟡 Minor comments (13)

massgen/tests/test_permission_coordinator.py-30-30 (1)

30-30: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove the unused feedback binding.

Line 30 unpacks feedback but never uses it, which matches the Ruff RUF059 warning and can fail the test/lint job. Rename it to _feedback or discard it in the tuple destructure.

🤖 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 `@massgen/tests/test_permission_coordinator.py` at line 30, The tuple unpack
from c.resolve_ask("a1", "execute_command", {"command": "python x.py"}) binds an
unused variable feedback; change the destructure to ignore it (e.g., use
_feedback or just _ ) so the unused binding is removed and the RUF059 lint
warning is resolved; update the call site in the test (the await c.resolve_ask
invocation) to only keep the used binding allowed and discard the second value.

Source: Linters/SAST tools

massgen/tests/test_permissions_optional.py-76-76 (1)

76-76: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Rename the unused agent bindings before Ruff blocks the file.

Line 76, Line 84, and Line 188 unpack agent but never use it, which matches the reported RUF059 warnings. Renaming those bindings to _agent or _ keeps the test intent unchanged and avoids a lint failure.

Also applies to: 84-84, 188-188

🤖 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 `@massgen/tests/test_permissions_optional.py` at line 76, The test unpacks an
unused binding `agent` from the _install(...) call causing RUF059; rename the
unused `agent` variable to `_agent` (or `_`) in the three calls that unpack
`agent, mgr = _install(...)` so the test intent remains the same and the linter
stops flagging RUF059; update the occurrences where `agent` is unused (the
`_install` unpack sites in this test file) to `_agent`/`_` while leaving `mgr`
and the `_install` call unchanged.

Source: Linters/SAST tools

massgen/tests/test_permissions_optional.py-33-202 (1)

33-202: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add Google-style docstrings to the new helpers and test functions.

This module adds a lot of new test surface, but the functions are undocumented. Please add short Google-style docstrings so the opt-in, parity, and persistence scenarios stay readable.

As per coding guidelines, **/*.py: For new or changed functions, include Google-style docstrings.

🤖 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 `@massgen/tests/test_permissions_optional.py` around lines 33 - 202, Add
concise Google-style docstrings to the new helper functions and each test
function in this module: document _install(config) and _engine(mgr) (describe
purpose, args, returns) and add short docstrings to all test_* functions
(one-line summary and any important fixtures/behavior) so the opt-in, backend
parity, and persistence scenarios are clear; ensure docstrings follow Google
style (Args, Returns where applicable) and update only the functions named
_install, _engine and the test_* functions present in the diff.

Source: Coding guidelines

massgen/tests/test_permission_denied_tool_visibility.py-20-95 (1)

20-95: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Document the new test cases with Google-style docstrings.

These tests cover several distinct deny-path behaviors, but none of the new functions carry the repo-required docstrings. Adding concise Google-style docstrings would make each scenario easier to maintain.

As per coding guidelines, **/*.py: For new or changed functions, include Google-style docstrings.

🤖 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 `@massgen/tests/test_permission_denied_tool_visibility.py` around lines 20 -
95, Add concise Google-style docstrings to each new test function
(test_denied_tool_emits_start_then_error_complete,
test_denied_tool_emission_is_safe_without_emitter,
test_denied_tool_emission_never_breaks_on_emitter_error,
test_preview_shows_the_command, test_preview_falls_back_to_path_then_tool_name)
describing the test purpose, setup, and expected outcome; place the docstring
immediately under the def line, use the Google style (short summary, optional
Args/Returns not required for tests), and keep them brief (one to three
sentences) to satisfy the repository docstring guideline for new/changed
functions.

Source: Coding guidelines

massgen/tests/test_permission_persistence.py-18-99 (1)

18-99: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add Google-style docstrings to the new helper and persistence tests.

These round-trip and edge-case tests are useful regression coverage, but the new functions are undocumented. Please add concise Google-style docstrings to keep each persistence scenario obvious.

As per coding guidelines, **/*.py: For new or changed functions, include Google-style docstrings.

🤖 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 `@massgen/tests/test_permission_persistence.py` around lines 18 - 99, Add
concise Google-style docstrings to the new helper and each persistence test to
satisfy the project's docstring policy: document the helper function _authz
(describe parameters tool, arguments, normalized_pattern and returned
AuthorizationObject) and add one-line Google-style docstrings to each test
function (test_persist_then_load_roundtrip_command,
test_persist_then_load_roundtrip_write_path,
test_persist_is_append_only_and_deduped,
test_persist_preserves_existing_settings,
test_persist_skips_when_no_stable_target, test_load_missing_file_returns_none,
test_load_corrupt_file_returns_none) describing the scenario they cover and
expected outcome; keep them short, describe inputs/state and expected behavior,
and place them immediately below each def signature.

Source: Coding guidelines

massgen/tests/test_srt_filesystem_integration.py-40-52 (1)

40-52: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a Google-style docstring to the new SRT wiring test.

The new read-only regression test is good coverage, but the changed function still needs the repo-required docstring.

As per coding guidelines, **/*.py: For new or changed functions, include Google-style docstrings.

🤖 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 `@massgen/tests/test_srt_filesystem_integration.py` around lines 40 - 52, Add a
Google-style docstring to the test function
test_read_only_flows_to_empty_allowwrite in
massgen/tests/test_srt_filesystem_integration.py: include a one-line summary, a
short description of what the test verifies (that a read-only role through
FilesystemManager with command_line_srt_read_only=True results in SRT settings
where filesystem.allowWrite == []), and any relevant args or setup notes
(tmp_path fixture). Keep it concise and follow the repo's Google-style docstring
format.

Source: Coding guidelines

massgen/tests/test_permission_guardrail_prompt.py-26-133 (1)

26-133: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add Google-style docstrings to the new helper and test functions.

The guardrail cases are clear, but the new functions are still missing the repo-required docstrings. Short Google-style docstrings would make the gating and rendering intent explicit.

As per coding guidelines, **/*.py: For new or changed functions, include Google-style docstrings.

🤖 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 `@massgen/tests/test_permission_guardrail_prompt.py` around lines 26 - 133, Add
short Google-style docstrings to each new helper/class/test function introduced:
_ChokepointBackend.__init__, _ChokepointBackend.set_permission_coordinator,
_NativeBackend.__init__, _render_with_gate, and all test functions (e.g.,
test_is_permissions_enabled_truth_table,
test_active_when_enabled_and_chokepoint_backend,
test_inactive_when_no_permissions_block, test_inactive_when_disabled,
test_inactive_on_native_backend_even_with_permissions,
test_section_states_the_core_policy,
test_section_encourages_the_approval_flow_not_just_blocks,
test_section_mentions_role_when_provided,
test_section_omits_role_line_when_absent,
test_prompt_includes_guardrails_when_active,
test_prompt_excludes_guardrails_when_inactive). Each docstring should follow
Google style: one-line summary, optional short elaboration, and for helpers
mention their purpose and key parameters/return behavior (e.g., that
guardrail_prompt_active(backend) checks permissions + chokepoint support,
PermissionGuardrailSection.render() returns the guardrail text), keeping them
brief.

Source: Coding guidelines

massgen/tests/test_tool_approval_modal.py-17-110 (1)

17-110: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add Google-style docstrings to the new modal and wiring tests.

This file introduces several new helpers and scenarios, but none of the functions have the required docstrings yet. Please add short Google-style docstrings so the approval-flow coverage stays easy to scan.

As per coding guidelines, **/*.py: For new or changed functions, include Google-style docstrings.

🤖 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 `@massgen/tests/test_tool_approval_modal.py` around lines 17 - 110, Several new
test helpers and test functions lack the required Google-style docstrings; add
concise Google-style docstrings to each new/changed callable to satisfy the
project guideline. Specifically, add short docstrings to the helper methods and
classes (_Disp.show_tool_approval_modal, _Backend.__init__,
_Backend.set_approval_provider, _Agent.__init__, _Orch.__init__, _ui) and to
each test function (test_decision_for_action_allow_scopes,
test_decision_for_action_reject_and_unknown_deny,
test_interactive_provider_installed_for_guarded_agent,
test_noop_in_automation_mode, test_noop_for_agent_without_coordinator,
test_noop_when_display_lacks_modal_method,
test_interactive_swap_does_not_override_file_provider) using the Google
docstring style: one-line summary, optional short description and
parameter/return sections where helpful; keep them brief and focused so test
coverage and readability requirements are met.

Source: Coding guidelines

massgen/tests/test_permission_hooks.py-14-80 (1)

14-80: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add Google-style docstrings to the new helper and test functions.

The new functions in this module are all undocumented right now. Please add short Google-style docstrings so each permission scenario is self-describing in the test output and source.

As per coding guidelines, **/*.py: For new or changed functions, include Google-style docstrings.

🤖 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 `@massgen/tests/test_permission_hooks.py` around lines 14 - 80, Add concise
Google-style docstrings to each new or changed function in this test module so
the behaviour is self-describing; specifically, add a one-line summary plus
optional short description and Args/Returns where appropriate for _args and for
each test function: test_hardline_blocks_catastrophic,
test_engine_low_risk_allows, test_engine_high_and_medium_ask,
test_engine_read_tool_allows, test_normalize_pattern,
test_allow_rule_suppresses_risk_ask, test_deny_rule_wins, and
test_no_rule_falls_through_to_risk; follow Google docstring style ("""Summary.
Optional further details. Args: ... Returns: ...""") and keep them short and
focused.

Source: Coding guidelines

massgen/system_prompt_sections.py-1953-1968 (1)

1953-1968: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Align changed methods with repository docstring and prompt-wrapping conventions.

PermissionGuardrailSection.__init__ (Line 1953) and PermissionGuardrailSection.build_content (Line 1961) are changed functions without Google-style method docstrings, and the prompt-line wrapping at Line 1967 uses implicit concatenation instead of the required escaped end-of-line continuation style for prompt E501 handling.

✍️ Proposed patch

 class PermissionGuardrailSection(SystemPromptSection):
@@
     def __init__(self, role: str | None = None):
+        """Initialize the permission guardrail prompt section.
+
+        Args:
+            role: Optional per-agent permission role to include in the policy.
+        """
         super().__init__(
             title="Permission Guardrails",
             priority=2,  # Safety policy: high, right after agent_identity(1)
             xml_tag="permission_guardrails",
         )
         self.role = role
 
     def build_content(self) -> str:
-        # Composed from short implicitly-concatenated fragments to keep source
-        # lines within the lint limit WITHOUT changing the rendered prompt
-        # (each bullet renders as one line, exactly as written).
+        """Build the permission guardrail policy content.
+
+        Returns:
+            The rendered policy text for the system prompt.
+        """
         role_line = ""
         if self.role:
-            role_line = f"\nYour assigned permission role is **'{self.role}'**, which " "further restricts what you may do.\n"
+            role_line = f"\nYour assigned permission role is **'{self.role}'**, which " \
+                "further restricts what you may do.\n"

As per coding guidelines, **/*.py requires Google-style docstrings for new/changed functions, and prompt E501 handling must preserve rendered text while using escaped line continuation (\) at end-of-line.

🤖 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 `@massgen/system_prompt_sections.py` around lines 1953 - 1968, The changed
PermissionGuardrailSection methods lack Google-style docstrings and use implicit
string concatenation for prompt wrapping; add proper Google-style docstrings to
PermissionGuardrailSection.__init__ and PermissionGuardrailSection.build_content
describing parameters and return types, and replace implicit concatenation in
build_content (where role_line and intro are composed) with explicit escaped
end-of-line continuations (use backslash "\" at line ends) so wrapped prompt
lines preserve rendered output while satisfying E501; update references in
docstrings to the class and the role parameter and ensure build_content's return
type and behavior are documented.

Source: Coding guidelines

docs/dev_notes/permission_systems_research.md-99-109 (1)

99-109: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a language to the fenced architecture block.

markdownlint flags this fence with MD040, which can fail docs lint/pre-commit. Use text/plaintext so the rendered content stays the same.

🤖 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 `@docs/dev_notes/permission_systems_research.md` around lines 99 - 109, The
fenced code block containing the architecture list (the triple-backtick block
starting with "PreToolUse hooks (subprocess/python/policy)   →   ALREADY:
GeneralHookManager") needs a language tag to satisfy markdownlint MD040; change
the opening fence from ``` to ```text or ```plaintext so rendering is unchanged
and the linter stops flagging MD040.

Source: Linters/SAST tools

docs/dev_notes/permissions_p2_followups.md-47-53 (1)

47-53: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Surround the run table with blank lines.

markdownlint reports MD058 here. Add a blank line before and after the table so docs lint passes consistently.

🤖 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 `@docs/dev_notes/permissions_p2_followups.md` around lines 47 - 53, Add a blank
line before the header "### Alignment ≠ enforcement — three live
`gemini-3-flash-preview` runs (all with permissions + guardrail prompt active)"
and another blank line after the Markdown table that begins with "| Run |
Outcome | What happened |" so the run table is separated by empty lines (this
fixes markdownlint MD058); ensure no other content touches those blank lines so
the table is properly delimited.

Source: Linters/SAST tools

massgen/backend/base_with_custom_tool_and_mcp.py-2783-2789 (1)

2783-2789: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Emit a concrete status for the approval-pending chunk.

ToolExecutionConfig has no status_running field, so this new chunk is emitted with status=None. That makes the approval-wait state unclassifiable for any downstream UI/status handling keyed on StreamChunk.status.

Suggested fix

                         yield StreamChunk(
                             type=config.chunk_type,
-                            status=getattr(config, "status_running", None),
+                            status="approval_pending",
                             content=f"⏸️ Awaiting approval for {tool_name}…",
                             source=f"{config.source_prefix}{tool_name}",
                             tool_call_id=call_id,
                         )

🤖 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 `@massgen/backend/base_with_custom_tool_and_mcp.py` around lines 2783 - 2789,
The emitted StreamChunk is given status=getattr(config, "status_running", None)
which yields None because ToolExecutionConfig has no status_running; replace
this with a concrete, discoverable status (e.g., use an existing
ToolExecutionConfig field like status_awaiting_approval or a canonical
enum/constant such as StreamChunkStatus.AWAITING_APPROVAL) so downstream UI can
classify the approval-pending state; update the ToolExecutionConfig to expose
that status field if needed and set StreamChunk(status=that_field) in the
StreamChunk yield (refer to StreamChunk, ToolExecutionConfig, config,
status_running, tool_name, call_id).

🧹 Nitpick comments (11)

massgen/tests/test_approval_ledger.py (1)

78-83: ⚡ Quick win

Assert cross-instance seq continuity, not just append count.

This test would still pass if a new ApprovalLedger instance reopened the same file, appended a second row, and reset seq back to 0. That weakens the audit trail because insertion order remains append-only but sequence numbers are no longer monotonic across writers. Please assert the written entries keep a strictly increasing seq across instances as well.

🤖 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 `@massgen/tests/test_approval_ledger.py` around lines 78 - 83, Update the
test_ledger_is_append_only_across_instances to assert seq continuity across
instances: after creating two ApprovalLedger instances and calling
ApprovalLedger(...).record(...), read and parse the two JSONL lines from the
file (created by ApprovalLedger.record), extract the "seq" fields from each
parsed object and assert that the later entry's seq is strictly greater than the
earlier one (e.g., second_seq > first_seq) to ensure sequence numbers are
monotonically increasing across instances rather than just checking line count.

massgen/tests/test_permission_coordinator.py (1)

91-139: ⚡ Quick win

Add a budget regression case for cached session grants.

These cases only debit the budget on provider-driven approvals. They never cover the other high-risk path in this PR: repeated GrantScope.SESSION cache hits for the same authorization. If cache-served approvals stop counting toward max_consecutive_auto, a runaway loop can bypass the breaker while this suite still stays green. Please add one test that primes a session grant and then repeats the same call until the cap trips.
As per coding guidelines, “After implementing any feature that involves passing parameters through multiple layers ... verify the full wiring chain end-to-end by tracing the parameter from its origin to its final usage site.”

🤖 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 `@massgen/tests/test_permission_coordinator.py` around lines 91 - 139, Add a
regression test that verifies SESSION-cache hits also consume the
ApprovalBudget: create a PermissionCoordinator with
PolicyApprovalProvider(AutomationDefault.ALLOW_ALL) and
ApprovalBudget(max_consecutive_auto= N) (use same symbol ApprovalBudget and
max_consecutive_auto), prime a GrantScope.SESSION grant for an agent via
resolve_ask (or by calling the provider so the session cache is populated), then
repeatedly call resolve_ask with the same agent/authorization to ensure after N
auto-approvals the budget trips and resolve_ask returns allowed=False and
feedback mentioning "budget"; reference PermissionCoordinator.resolve_ask,
PolicyApprovalProvider, GrantScope.SESSION, and ApprovalBudget to locate the
wiring to ensure session-cache approvals decrement the same budget counter.

Source: Coding guidelines

massgen/tests/test_srt_manager.py (1)

134-140: ⚡ Quick win

Broaden the fs_tools assertion under read_only=True.

This only proves workspace stays writable for fs_tools. It won’t catch a regression where read_only=True strips the temp workspace or extra snapshot paths from the fs_tools profile, which would break snapshotting while this test still passes. Please mirror the full fs_tools write contract here, not just the workspace entry.

🤖 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 `@massgen/tests/test_srt_manager.py` around lines 134 - 140, The test
test_read_only_empties_execution_writes_but_keeps_fs_tools should assert the
full fs_tools write contract instead of only the workspace entry; update the
test to build an expected allowWrite list from pm_with_paths (include
pm_with_paths["workspace"] plus the temp/workspace snapshot paths provided by
the fixture such as pm_with_paths["temp_workspace"] and any snapshot path
entries) and assert
mgr.build_settings(profile="fs_tools")["filesystem"]["allowWrite"] equals (or
contains) that full expected list using SrtManager and build_settings to locate
the value.

massgen/frontend/displays/textual/widgets/modals/tool_approval_modal.py (1)

59-91: ⚡ Quick win

Consider adding Google-style docstrings to methods.

The coding guidelines specify that new functions should include Google-style docstrings. Methods like __init__, compose, on_button_pressed, and the action handlers would benefit from brief docstrings explaining their purpose and parameters.

Example for __init__:

def __init__(self, authz: AuthorizationObject, **kwargs) -> None:
    """Initialize the tool approval modal.
    
    Args:
        authz: Authorization object containing tool call details to display.
        **kwargs: Additional arguments passed to ModalScreen.
    """

As per coding guidelines: "**/*.py: For new or changed functions, include Google-style docstrings"

🤖 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 `@massgen/frontend/displays/textual/widgets/modals/tool_approval_modal.py`
around lines 59 - 91, Add Google-style docstrings to the methods in
ToolApprovalModal: __init__, compose, on_button_pressed, action_reject,
action_allow_once, action_allow_session, and action_allow_always; each docstring
should be a brief one- or two-line description plus an Args section for
parameters (e.g., authz: AuthorizationObject and **kwargs for __init__, event:
Button.Pressed for on_button_pressed) and a Returns section when applicable
(e.g., None), matching the project's Google-style docstring format.

Source: Coding guidelines

massgen/frontend/displays/textual_terminal_display.py (1)

2409-2415: ⚡ Quick win

Use a Google-style docstring for the new async API.

This docstring explains intent, but it doesn't follow the repo's required Google-style format for changed Python functions. Please add Args, Returns, and the fail-closed behavior so the display/provider contract is explicit. As per coding guidelines, **/*.py: "For new or changed functions, include Google-style docstrings".

🤖 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 `@massgen/frontend/displays/textual_terminal_display.py` around lines 2409 -
2415, Update the docstring for async method show_tool_approval_modal to use
Google-style format: add an "Args" section describing the authz parameter and
its type/meaning, a "Returns" section specifying it returns an ApprovalDecision
(or "Any" per signature) and what values mean, and explicitly document the
fail-closed behavior (on no-app / error / timeout the method returns a
closed/deny decision). Keep the existing intent text brief and ensure the
docstring follows the repository's Google-style sections for new/changed
functions.

Source: Coding guidelines

massgen/frontend/coordination_ui.py (1)

815-818: ⚡ Quick win

Use a Google-style docstring for the new helper.

Add an Args: section for orchestrator so this new method matches the repo's Python docstring contract. As per coding guidelines, **/*.py: For new or changed functions, include Google-style docstrings.

🤖 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 `@massgen/frontend/coordination_ui.py` around lines 815 - 818, The method
_install_interactive_approval_provider currently uses a short triple-quoted
description; replace it with a Google-style docstring including a one-line
summary, a blank line, and an Args: section documenting the orchestrator
parameter (type and purpose) so the helper conforms to the repo's docstring
contract; ensure the docstring remains on the
_install_interactive_approval_provider method and mentions that it is a no-op in
headless/automation and for agents without a permission coordinator.

Source: Coding guidelines

massgen/configs/tools/permissions/per_agent_roles.yaml (1)

20-21: ⚡ Quick win

Use cost-effective default models across the permission demo configs.

massgen/configs/tools/permissions/per_agent_roles.yaml and massgen/configs/tools/permissions/permission_engine.yaml are both example configs for exercising permissions behavior, but they currently default to gpt-5. Swapping these demos to gpt-5-mini, gpt-5-nano, or gemini-2.5-flash would match the repo convention and reduce accidental spend for users copying the examples. As per coding guidelines, massgen/configs/**/*.yaml: Prefer cost-effective models (gpt-5-nano, gpt-5-mini, gemini-2.5-flash).

🤖 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 `@massgen/configs/tools/permissions/per_agent_roles.yaml` around lines 20 - 21,
The YAML demo currently sets model: "gpt-5" which is expensive; update the
example configs to use a cost-effective default (e.g., "gpt-5-mini",
"gpt-5-nano", or "gemini-2.5-flash") in the permission demo files so users don't
incur accidental spend — locate the model key in
massgen/configs/tools/permissions/per_agent_roles.yaml (type: "openai", model:
"gpt-5") and the analogous model setting in
massgen/configs/tools/permissions/permission_engine.yaml and replace "gpt-5"
with one of the approved cheap defaults.

Source: Coding guidelines

massgen/permissions/risk_classifier.py (1)

48-87: ⚡ Quick win

Add Google-style docstrings to the new permission helpers.

These new functions/methods are missing Google-style docstrings (or only have terse one-liners), which breaks the repo’s Python documentation rule on a new public subsystem. Please add proper Args / Returns / Raises sections where applicable so the permission API is documented consistently.

As per coding guidelines, "**/*.py: For new or changed functions, include Google-style docstrings."

🤖 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 `@massgen/permissions/risk_classifier.py` around lines 48 - 87, The new public
permission helper class RiskClassifier and its methods _command_text and
classify lack Google-style docstrings; add proper Google-style docstrings for
the class and for each method including Args and Returns (and Raises if any
exceptions can be raised) so the permission API follows repo rules—document
RiskClassifier (purpose), _command_text(arguments: dict[str, Any]) with Args
describing 'arguments' and the returned str, and classify(tool: str, arguments:
dict[str, Any]) with Args for 'tool' and 'arguments' and Returns describing
RiskTier values (and note any side effects), keeping phrasing consistent with
existing docstring style in the repo.

Source: Coding guidelines

massgen/backend/base_with_custom_tool_and_mcp.py (1)

571-582: ⚡ Quick win

Convert the new helper docstrings to Google style.

These are all new/changed Python methods, but their docstrings are narrative-only. The repo standard requires Google-style docstrings for changed Python functions so Args/Returns stay consistent in generated docs and reviews.

As per coding guidelines, “**/*.py: For new or changed functions, include Google-style docstrings”.

Also applies to: 2501-2550

🤖 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 `@massgen/backend/base_with_custom_tool_and_mcp.py` around lines 571 - 582,
Update the narrative docstrings for set_permission_coordinator and
set_approval_provider to Google-style docstrings: add a short summary line, an
"Args:" section documenting the parameter (coordinator: Any for
set_permission_coordinator; provider: Any for set_approval_provider), and a
"Returns:" section (None) if needed; preserve the existing descriptions about
behavior (how ask resolves and behaviour when unset, and swapping the
ApprovalProvider) inside the summary or expanded description so generated docs
follow the repo's Google-style docstring convention.

Source: Coding guidelines

massgen/backend/base.py (1)

229-233: ⚡ Quick win

Extract the computed read-only flag for readability and line length.

Line 232 (~175 chars) is difficult to scan and likely triggers E501. Consider extracting to a local variable above the dict literal:

♻️ Suggested refactor

+                # A read-only permission role also empties the SRT writable set
+                # (OS-layer backstop to the permission engine's write denials).
+                permissions_cfg = kwargs.get("permissions")
+                srt_read_only = (
+                    isinstance(permissions_cfg, dict)
+                    and str(permissions_cfg.get("role", "")).lower() in ("read-only", "researcher")
+                )
+
                 # Extract all FilesystemManager parameters from kwargs
                 filesystem_params = {
                     "cwd": cwd,
                     ...
-                    # A read-only permission role also empties the SRT writable set
-                    # (OS-layer backstop to the permission engine's write denials).
-                    "command_line_srt_read_only": (
-                        str((kwargs.get("permissions") or {}).get("role", "")).lower() in ("read-only", "researcher") if isinstance(kwargs.get("permissions"), dict) else False
-                    ),
+                    "command_line_srt_read_only": srt_read_only,

This also removes the redundant or {} (already guarded by the isinstance check).

🤖 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 `@massgen/backend/base.py` around lines 229 - 233, The long inline computation
for "command_line_srt_read_only" makes the dict literal hard to read and may
violate line-length rules; extract the logic into a local boolean variable
(e.g., command_line_srt_read_only = isinstance(kwargs.get("permissions"), dict)
and str(kwargs["permissions"].get("role","")).lower() in
("read-only","researcher")) placed above the dict, then reference that variable
in the dict value and remove the redundant or {} in the original expression.

massgen/permissions/approval_provider.py (1)

27-30: ⚡ Quick win

Add Google-style docstrings to the new approval-provider callables.

The new public call surface here (ApprovalProvider.request_approval, the provider constructors, FileApprovalProvider.request_id, and the request methods) still only has one-liners or no function-level docstrings. Since this module is now a shared permission API, the signatures should carry Args / Returns / behavior details directly on the functions.

As per coding guidelines, **/*.py: For new or changed functions, include Google-style docstrings.

Also applies to: 37-40, 88-94, 155-165

🤖 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 `@massgen/permissions/approval_provider.py` around lines 27 - 30, Add
Google-style docstrings to the new public call surface: include full docstrings
(brief summary, Args, Returns, and behavior/side-effects) for
ApprovalProvider.request_approval, each provider constructor,
FileApprovalProvider.request_id, and the async request methods (e.g., the
concrete provider request_approval implementations). For each function/method
(including the class constructors) describe the expected input types and
meanings under Args, the returned value under Returns (including type
ApprovalDecision or similar), any exceptions or error conditions raised, and any
important behavior (e.g., IO/network side-effects, blocking/async expectations,
caching or persistence). Ensure the docstrings follow Google-style formatting
and are added to the functions/methods named request_approval and request_id and
to provider constructors so the public API is self-documented.

Source: Coding guidelines

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b1867e74-a893-46a5-b088-86e143484f9d

📥 Commits

Reviewing files that changed from the base of the PR and between 3caf966 and 2b119fe.

📒 Files selected for processing (42)

• docs/dev_notes/permission_systems_research.md
• docs/dev_notes/permissions_p2_followups.md
• massgen/backend/_excluded_params.py
• massgen/backend/base.py
• massgen/backend/base_with_custom_tool_and_mcp.py
• massgen/configs/tools/permissions/per_agent_roles.yaml
• massgen/configs/tools/permissions/permission_engine.yaml
• massgen/configs/tools/permissions/permission_modal_interactive.yaml
• massgen/filesystem_manager/_filesystem_manager.py
• massgen/filesystem_manager/_srt_manager.py
• massgen/frontend/coordination_ui.py
• massgen/frontend/displays/textual/widgets/modals/tool_approval_modal.py
• massgen/frontend/displays/textual_terminal_display.py
• massgen/orchestrator_collaborators/midstream_injection_hook_installer.py
• massgen/permissions/__init__.py
• massgen/permissions/activation.py
• massgen/permissions/approval_provider.py
• massgen/permissions/coordinator.py
• massgen/permissions/hardline.py
• massgen/permissions/hooks.py
• massgen/permissions/ledger.py
• massgen/permissions/models.py
• massgen/permissions/persistence.py
• massgen/permissions/risk_classifier.py
• massgen/permissions/rules.py
• massgen/permissions/session_cache.py
• massgen/system_message_builder.py
• massgen/system_prompt_sections.py
• massgen/tests/test_approval_ledger.py
• massgen/tests/test_approval_provider.py
• massgen/tests/test_file_approval_provider.py
• massgen/tests/test_permission_coordinator.py
• massgen/tests/test_permission_denied_tool_visibility.py
• massgen/tests/test_permission_guardrail_prompt.py
• massgen/tests/test_permission_hooks.py
• massgen/tests/test_permission_persistence.py
• massgen/tests/test_permission_rules.py
• massgen/tests/test_permissions_core.py
• massgen/tests/test_permissions_optional.py
• massgen/tests/test_srt_filesystem_integration.py
• massgen/tests/test_srt_manager.py
• massgen/tests/test_tool_approval_modal.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Also exclude command_line_srt_read_only from provider payloads.

This canonical set is being updated for permissions, but the new local-only command_line_srt_read_only backend flag introduced in FilesystemManager is still missing from the neighboring command_line_srt_* entries. Because both backend and API parameter forwarding derive from this set, that flag will be forwarded to provider APIs as an unexpected request field unless it's added here too.

As per coding guidelines, massgen/**/*.yaml: When adding new YAML parameters, update both massgen/backend/base.py -> get_base_excluded_config_params() and massgen/api_params_handler/_api_params_handler_base.py -> get_base_excluded_config_params().

🤖 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 `@massgen/backend/_excluded_params.py` around lines 111 - 112, Add
"command_line_srt_read_only" to the canonical excluded params list so it is not
forwarded to providers; update the same exclusion set used by
get_base_excluded_config_params() in both places that maintain the canonical
list (the backend/base get_base_excluded_config_params and the
api_params_handler/_api_params_handler_base get_base_excluded_config_params) to
include "command_line_srt_read_only" alongside the existing "permissions" entry
so the new local-only FilesystemManager flag is treated as excluded.

Source: Coding guidelines

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve the real tool arguments through the approval path.

This branch parses arguments_str with json.loads(...), but elsewhere this backend explicitly supports dict inputs and multi-pass normalization. Here that means _approval_args / _denied_args can collapse to {} for already-parsed dicts or double-encoded JSON, so the approval provider and denied preview lose the actual command/path being approved or rejected.

Suggested fix

-                        try:
-                            _approval_args = json.loads(arguments_str) if arguments_str else {}
-                        except (json.JSONDecodeError, TypeError):
-                            _approval_args = {}
+                        try:
+                            _approval_args = self._parse_tool_arguments(arguments_str)
+                        except ValueError:
+                            _approval_args = {}
+                        if not isinstance(_approval_args, dict):
+                            _approval_args = {}
...
-                    try:
-                        _denied_args = json.loads(arguments_str) if arguments_str else {}
-                    except (json.JSONDecodeError, TypeError):
-                        _denied_args = {}
+                    try:
+                        _denied_args = self._parse_tool_arguments(arguments_str)
+                    except ValueError:
+                        _denied_args = {}
                     if not isinstance(_denied_args, dict):
                         _denied_args = {}

As per coding guidelines, “For cross-backend tool-calling behavior … treat backend-side argument normalization and schema validation as the source of truth … tolerate/detect accidentally stringified JSON argument payloads in adapters and tool gateways.”

🤖 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 `@massgen/backend/base_with_custom_tool_and_mcp.py` around lines 2779 - 2809,
The approval/deny branch currently unconditionally json.loads(arguments_str)
which collapses already-parsed dicts or double-encoded payloads to {} — preserve
the real argument object instead: if arguments_str is already a dict/object use
it directly for _approval_args/_denied_args; if it's a string attempt json.loads
and on failure or if the result isn't a dict fall back to the original value
(and only coerce to {} as a last resort), then pass that preserved dict-like
value into coordinator.resolve_ask(...) and into
self._denied_tool_preview(tool_name, _denied_args); ensure type checks
(isinstance(..., dict)) and avoid swallowing non-dict payloads earlier in this
flow.

Source: Coding guidelines

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

The demo walkthrough targets a different action than the ask rule.

The instructions tell users to trigger the modal with curl ..., but the config only marks read_url(*) as ask. A shell curl goes through the command-execution path, so this demo does not reliably exercise the modal flow it advertises. Either change the walkthrough to use a real read_url action or add an explicit command(curl *)/equivalent ask rule for the showcased command.

Also applies to: 39-45

🤖 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 `@massgen/configs/tools/permissions/permission_modal_interactive.yaml` around
lines 9 - 18, The walkthrough triggers a shell `curl` but the config's `ask`
rule targets `read_url(*)`, so the modal isn't exercised; either update the demo
text to perform an actual `read_url(...)` action (so the example command matches
`read_url(*)`) or add an explicit `ask` rule for the command path such as
`command(curl *)` (or the equivalent command execution pattern used by your
permission matcher) so the shown `curl` invocation will hit the approval modal;
update both the top example and the repeated block at lines ~39-45 to keep them
consistent with the `read_url(*)` vs `command(curl *)` change.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't swallow permission import failures here.

If either massgen.permissions.* import breaks, this broad except silently leaves the coordinator on PolicyApprovalProvider. In an interactive run that can turn an ask back into automation-policy behavior instead of showing the approval modal.

Suggested fix

-        except Exception:
-            return
+        except ImportError:
+            logger.exception("[TUI] Failed to import interactive approval dependencies")
+            return

🧰 Tools

🪛 Ruff (0.15.15)

[warning] 830-830: Do not catch blind exception: Exception

(BLE001)

🤖 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 `@massgen/frontend/coordination_ui.py` around lines 824 - 831, The current
broad except silently swallows any error when importing
CallbackApprovalProvider, PolicyApprovalProvider, and ApprovalDecision which can
leave the coordinator stuck using PolicyApprovalProvider; change the handler to
only catch ImportError (or ModuleNotFoundError) so legitimate import failures
are handled while other exceptions propagate, or log and re-raise non-import
exceptions; update the try/except around the imports of
CallbackApprovalProvider, PolicyApprovalProvider, and ApprovalDecision
accordingly so you don't return silently on unexpected errors.

Source: Linters/SAST tools

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="massgen/frontend/displays/textual_terminal_display.py"

# Show the full function area with line numbers
sed -n '2360,2520p' "$FILE" | nl -ba | sed -n '1,220p'

# Find the function definition line(s)
rg -n "def show_tool_approval_modal|show_tool_approval_modal" "$FILE"

# Search for similar modal push/pop patterns elsewhere
rg -n "push_screen\\(|pop_screen\\(|push_screen\\s*\\(" massgen/frontend/displays/textual_terminal_display.py
rg -n "push_screen\\(|pop_screen\\(|call_from_thread\\(" massgen/frontend/displays/textual_terminal_display.py

# Inspect any other files for the same modal or pattern
rg -n "ToolApprovalModal" massgen/frontend/displays/textual_terminal_display.py massgen/frontend -S || true
rg -n "call_from_thread\\(" massgen/frontend/displays -S || true

# Get ToolApprovalModal definition if present nearby or imported
rg -n "class ToolApprovalModal|ToolApprovalModal\\(" massgen/frontend/displays/textual_terminal_display.py massgen/frontend -S || true

Repository: massgen/MassGen

Length of output: 102

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="massgen/frontend/displays/textual_terminal_display.py"

sed -n '2360,2520p' "$FILE" | nl -ba | sed -n '1,220p'

rg -n "def show_tool_approval_modal|show_tool_approval_modal" "$FILE"

rg -n "push_screen\\(|pop_screen\\(|call_from_thread\\(" massgen/frontend/displays/textual_terminal_display.py

rg -n "push_screen\\(|pop_screen\\(|call_from_thread\\(" massgen/frontend/displays -S || true

rg -n "ToolApprovalModal" massgen/frontend/displays/textual_terminal_display.py massgen/frontend -S || true

rg -n "class ToolApprovalModal|ToolApprovalModal\\(" massgen/frontend/displays/textual_terminal_display.py massgen/frontend -S || true

Repository: massgen/MassGen

Length of output: 102

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="massgen/frontend/displays/textual_terminal_display.py"

sed -n '2360,2520p' "$FILE" | nl -ba | sed -n '1,220p'

rg -n "def show_tool_approval_modal|show_tool_approval_modal" "$FILE"

rg -n "push_screen\\(|pop_screen\\(|call_from_thread\\(" massgen/frontend/displays/textual_terminal_display.py

rg -n "push_screen\\(|pop_screen\\(|call_from_thread\\(" massgen/frontend/displays -S || true

rg -n "ToolApprovalModal" massgen/frontend/displays/textual_terminal_display.py massgen/frontend -S || true

rg -n "class ToolApprovalModal|ToolApprovalModal\\(" massgen/frontend/displays/textual_terminal_display.py massgen/frontend -S || true

Repository: massgen/MassGen

Length of output: 102

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="massgen/frontend/displays/textual_terminal_display.py"

# Show the full function area with line numbers
sed -n '2360,2520p' "$FILE" | nl -ba | sed -n '1,220p'

# Find the function definition line(s)
rg -n "def show_tool_approval_modal|show_tool_approval_modal" "$FILE"

# Search for push/pop/call patterns in this file and nearby displays code
rg -n "push_screen\\(|pop_screen\\(|call_from_thread\\(" "$FILE"
rg -n "push_screen\\(|pop_screen\\(|call_from_thread\\(" massgen/frontend/displays -S || true

# Find ToolApprovalModal definition/usages
rg -n "ToolApprovalModal" massgen/frontend/displays/textual_terminal_display.py massgen/frontend -S || true
rg -n "class ToolApprovalModal|ToolApprovalModal\\(" massgen/frontend/displays/textual_terminal_display.py massgen/frontend -S || true

Repository: massgen/MassGen

Length of output: 102

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="massgen/frontend/displays/textual_terminal_display.py"

# Show the target region with line numbers (no nl dependency)
sed -n '2360,2520p' "$FILE" | cat -n

# Function definition location
rg -n "def show_tool_approval_modal" "$FILE" || true
rg -n "show_tool_approval_modal" "$FILE" || true

# Modal push/pop/threading usage in this area/file
rg -n "push_screen\\(|pop_screen\\(|call_from_thread\\(" "$FILE" || true

# Look for ToolApprovalModal definition/usages
rg -n "ToolApprovalModal" "$FILE" || true
rg -n "class ToolApprovalModal" "$FILE" || true

# Check for exception handling patterns (CancelledError etc.)
rg -n "CancelledError|wait_for\\(|asyncio\\.wait_for\\(" "$FILE" || true

Repository: massgen/MassGen

Length of output: 10989

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
FILE="massgen/frontend/displays/textual_terminal_display.py"

# Show show_change_review_modal and its docstring/cleanup patterns (nearby references from earlier rg output)
sed -n '2300,2480p' "$FILE" | cat -n

# Show other push_screen/pop_screen patterns in the same file for lifecycle consistency
sed -n '1800,1935p' "$FILE" | cat -n

# Show any other modal-wait/timeout implementations for CancelledError handling
sed -n '2360,2565p' "$FILE" | cat -n

# Narrow search: where pop_screen is called, capture small contexts
rg -n "call_from_thread\\(self\\._app\\.pop_screen" "$FILE"
rg -n "pop_screen\\(\\)" "$FILE"

Repository: massgen/MassGen

Length of output: 26095

---

Track/dismiss the exact ToolApprovalModal on timeout and task cancellation

In TextualTerminalDisplay.show_tool_approval_modal, the code pushes the modal via push_screen(modal, ...) but dismisses it on timeout with a bare self._app.pop_screen(), which can pop the wrong screen if the stack changes. Also, if the awaiting coroutine is cancelled, the TimeoutError handler won’t run, so the modal may remain mounted. Store the specific modal reference (like other modal code in this file does) and only pop/dismiss when the top screen is that exact modal; handle asyncio.CancelledError (or use a finally) to ensure cleanup.

Additionally, update the show_tool_approval_modal docstring to Google-style (include Args: / Returns:), matching the repo’s docstring guideline for new/changed functions.

🧰 Tools

🪛 Ruff (0.15.15)

[warning] 2440-2440: Do not catch blind exception: Exception

(BLE001)

---

[warning] 2447-2447: Do not catch blind exception: Exception

(BLE001)

---

[error] 2457-2458: try-except-pass detected, consider logging the exception

(S110)

---

[warning] 2457-2457: Do not catch blind exception: Exception

(BLE001)

🤖 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 `@massgen/frontend/displays/textual_terminal_display.py` around lines 2428 -
2459, The show_tool_approval_modal flow pushes a ToolApprovalModal but dismisses
it with a bare self._app.pop_screen and doesn't handle coroutine cancellation;
fix TextualTerminalDisplay.show_tool_approval_modal by storing the modal
instance (modal = ToolApprovalModal(...)) and, on timeout or cancellation (catch
asyncio.TimeoutError and asyncio.CancelledError or use a finally block), call
self._app.call_from_thread to dismiss only if the current top screen is that
exact modal (check identity/equality against the stored modal) to avoid popping
the wrong screen, ensure result_future is set if still pending, and update the
function docstring to Google-style with Args: and Returns: sections.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Guard the settings write against concurrent lost updates.

This is a plain read-modify-write of settings.local.json, so two overlapping always approvals can race: both read the same old file, append different rules, and the later write drops the earlier grant. Because this PR adds multi-agent approval persistence, the durable allowlist needs either a file lock plus reread/merge or an atomic write path guarded by a process-level lock.

🧰 Tools

🪛 ast-grep (0.43.0)

[info] 71-71: use jsonify instead of json.dumps for JSON output
Context: json.dumps(data, indent=2)
Note: Security best practice.

(use-jsonify)

🤖 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 `@massgen/permissions/persistence.py` around lines 58 - 72, The
read-modify-write around _read_settings / target_path that appends rule to
permissions.rules.allow is vulnerable to race conditions; wrap this logic in a
cross-process file lock, then once the lock is held re-read the file (call
_read_settings again), merge/union the allow list (dedupe), and perform an
atomic write (write to a temp file in the same directory then os.replace) so
concurrent approvals are not lost; ensure you still create
target_path.parent.mkdir(parents=True, exist_ok=True) before the atomic rename
and release the lock after the replace.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Replace tool-name heuristic routing in the permission engine.

This risk path is driven by substring lists on tool names (_COMMAND_TOOL_HINTS, _READ_TOOL_HINTS, etc.), so permission decisions will drift as soon as a provider renames a tool or a new tool happens to share one of these substrings. That is a brittle safety boundary for an allow/ask/deny engine. Move this to schema-backed capability metadata or an LLM-backed classifier layered on top of the deterministic hardline floor instead of keyword heuristics.

As per coding guidelines, "Avoid writing code that infers categories, detects similarity, or organizes content using keyword lists, Jaccard similarity, Levenshtein distance, or similar heuristics. Use LLM-based approaches instead" and "No keyword/heuristic matching for categorization or similarity."

Also applies to: 56-84

🤖 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 `@massgen/permissions/risk_classifier.py` around lines 21 - 24, The constants
_COMMAND_TOOL_HINTS, _NETWORK_TOOL_HINTS, _READ_TOOL_HINTS, and
_WRITE_TOOL_HINTS implement brittle substring heuristics; replace this by
reading explicit capability metadata from each tool's schema (e.g., a
Tool.capabilities or tool_schema.capabilities field) inside
massgen.permissions.risk_classifier and using that to make deterministic
allow/deny decisions, and if needed layer an LLM-backed classifier (called only
as a fallback) for ambiguous/missing metadata; remove the hardcoded hint tuples
and update any functions in risk_classifier that reference them to consult tool
metadata (and/or call the new LLM classifier) so permission logic relies on
declared capabilities rather than keyword matching.

Source: Coding guidelines

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

read-only can still mutate through generic MCP tools.

This classifier infers capabilities from tool-name substrings and otherwise falls back to mcp(server/tool). That means any side-effecting MCP/tool that misses these hint lists bypasses the write_file(*) / command(*) denies, so the read-only and researcher presets are not actually read-only for non-filesystem mutations. Push an explicit action/capability from tool registration/backend adapters instead of deriving it from keyword heuristics.

As per coding guidelines, “No keyword/heuristic matching for categorization or similarity” and “Use LLM-based approaches instead -- that's the whole point of this project.”

Also applies to: 171-186

Source: Coding guidelines

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve the cached grant scope instead of collapsing it to bool.

grant() stores GrantScope.ALWAYS, but check() only returns True/False. The supplied PermissionCoordinator.resolve_ask() snippet then records every cache hit as GrantScope.SESSION, so repeated uses of an always approval are mis-audited as session-scoped approvals. Return the stored scope (or (hit, scope)) here so the coordinator can keep the ledger accurate.

Suggested direction

-    def check(self, key: str) -> bool:
-        """Return True if a grant covers this key. Consumes a ONCE grant."""
-        scope = self._grants.get(key)
-        if scope is None:
-            return False
-        if scope == GrantScope.ONCE:
-            del self._grants[key]
-        return True
+    def check(self, key: str) -> GrantScope | None:
+        """Return the covering grant scope. Consumes a ONCE grant."""
+        scope = self._grants.get(key)
+        if scope is None:
+            return None
+        if scope == GrantScope.ONCE:
+            del self._grants[key]
+        return scope

A matching coordinator change is then needed so audit entries use the returned scope instead of hard-coding GrantScope.SESSION.

🤖 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 `@massgen/permissions/session_cache.py` around lines 23 - 34, The check method
currently collapses stored GrantScope to a bool; change SessionCache.check so it
returns the stored GrantScope (or a tuple like (hit: bool, scope: GrantScope))
instead of just True/False, preserving GrantScope.ALWAYS and still removing
GrantScope.ONCE entries when consumed; update callers (notably
PermissionCoordinator.resolve_ask) to accept the new return shape and record the
exact scope from SessionCache.check in the audit/ledger instead of hard-coding
GrantScope.SESSION.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Apply the permission guardrail to presentation and post-evaluation prompts too.

This only injects PermissionGuardrailSection in build_coordination_message(). build_presentation_message() and build_post_evaluation_message() still expose tool/file execution paths without the same anti-circumvention prompt, so the “guardrail when permissions are active” contract becomes phase-dependent. Please factor this gate into a shared helper and call it from all three builders.

🤖 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 `@massgen/system_message_builder.py` around lines 262 - 271, The permission
guardrail logic (guardrail_prompt_active(agent.backend) check, extracting
_perms/_role, calling builder.add_section(PermissionGuardrailSection(...)) and
logging) is only used in build_coordination_message; factor it into a shared
helper (e.g. apply_permission_guardrail or _add_permission_guardrail_section)
and call that helper from build_coordination_message,
build_presentation_message, and build_post_evaluation_message so the
PermissionGuardrailSection is injected consistently; the helper should import
and use guardrail_prompt_active and add the same logger.info line referencing
agent_id and _role and preserve the existing behavior of only adding the section
when permissions are active.