Skip to content

fix(security): require explicit trust for local scan config#714

Open
mldangelo wants to merge 9 commits intomainfrom
feat/trusted-local-config
Open

fix(security): require explicit trust for local scan config#714
mldangelo wants to merge 9 commits intomainfrom
feat/trusted-local-config

Conversation

@mldangelo
Copy link
Member

@mldangelo mldangelo commented Mar 16, 2026

Summary

  • stop auto-applying local .modelaudit.toml and pyproject.toml scan policy by default
  • prompt interactive text scans to use a detected local config once or remember trust securely in the cache
  • invalidate remembered trust when the config changes and keep the flow disabled in CI and delegated wrapper runs

Testing

  • uv run ruff format --check modelaudit/ tests/
  • uv run ruff check modelaudit/ tests/
  • uv run mypy modelaudit/
  • uv run pytest -n auto -m "not slow and not integration" --maxfail=1

Summary by CodeRabbit

  • Security Improvements

    • Local policy files no longer auto-apply during scans; trusted entries are stored locally and invalidated if the config changes.
    • Non-interactive/CI runs must use explicit configuration.
  • New Features

    • Interactive trust flow to remember or decline local policies; applying a trusted local policy disables per-run result caching and shows a notice.
    • Local config discovery for scan paths.
  • Documentation

    • Docs updated to explain local scan policy, trust mechanics, and CI guidance.
  • Tests

    • Added tests covering discovery, trust/untrust flows, invalidation, and cache behavior.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 16, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Local .modelaudit.toml and pyproject.toml policy files are no longer auto-applied during scans; a discovered local config must be explicitly trusted in an interactive session. Trusted entries are stored in a secure local ModelAudit cache and are invalidated if the config file changes.

Changes

Cohort / File(s) Summary
Documentation
CHANGELOG.md, docs/user/security-model.md
Document new local policy behavior: local policy files not auto-applied; interactive trust can be granted and remembered; remembered trust stored in local cache and invalidated on config change; advise CI/non-interactive runs to use explicit config.
Trusted Config Storage
modelaudit/cache/trusted_config_store.py
New TrustedConfigRecord and TrustedConfigStore: versioned JSON trust store with SHA-256 hashing, secure path/symlink checks, atomic writes with tightened permissions, load/validate/is_trusted/trust APIs.
Local Config Discovery
modelaudit/config/local_config.py
New LocalConfigCandidate dataclass and find_local_config_for_paths() walking parents to locate a shared .modelaudit.toml or pyproject.toml containing [tool.modelaudit]; resilient to missing files and parse errors.
Configuration Management
modelaudit/config/rule_config.py
Added ModelAuditConfig.copy(), load(..., discover_local: bool) to control local discovery, and from_cli_args(..., base_config) to merge CLI overrides onto an optional base config; get_config() now calls load(discover_local=False).
CLI Integration
modelaudit/cli.py
Added helpers: get_trusted_config_store(), can_use_trusted_local_config(), maybe_load_trusted_local_config(), build_scan_rule_config(); integrated interactive trusted-local-config flow into scan command and disable scan cache when a local config is applied.
Tests
tests/test_cli.py, tests/test_rules.py
Added/updated tests for trusted/untrusted local config handling, interactive trust flow, persistence/invalidation of trusted entries, cache disablement when local config applied, pyproject discovery, CLI base_config merging, and trust-store edge cases (config change, symlink).

Sequence Diagram

sequenceDiagram
    participant User as User
    participant CLI as CLI
    participant Finder as LocalConfigFinder
    participant Store as TrustedConfigStore
    participant Loader as ConfigLoader

    User->>CLI: run scan
    CLI->>Finder: find_local_config_for_paths(paths)
    Finder-->>CLI: LocalConfigCandidate / None

    alt candidate found
        CLI->>Store: is_trusted(candidate)
        Store-->>CLI: true / false

        alt already trusted
            CLI->>Loader: load and apply local config
            Loader-->>CLI: ModelAuditConfig
        else not trusted
            alt interactive session
                CLI->>User: prompt to trust config?
                User-->>CLI: yes / no

                alt yes
                    CLI->>Store: trust(candidate)
                    Store-->>CLI: stored
                    CLI->>Loader: load and apply local config
                    Loader-->>CLI: ModelAuditConfig
                else no
                    CLI->>Loader: use default config
                    Loader-->>CLI: ModelAuditConfig
                end
            else non-interactive
                CLI->>Loader: use default config
                Loader-->>CLI: ModelAuditConfig
            end
        end
    else no candidate
        CLI->>Loader: use default config
        Loader-->>CLI: ModelAuditConfig
    end

    CLI->>User: run scan with chosen config
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 I sniffed the tomls on the lane,

“Trust?” I asked, with whiskers twain.
I hashed each leaf, I kept the true,
Tossed broken links that bungled through.
Now scans hop safe — a carrot-cheer for you!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and specifically describes the main security-focused change: preventing automatic application of local config files unless explicitly trusted by the user.
Docstring Coverage ✅ Passed Docstring coverage is 95.24% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/trusted-local-config
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@modelaudit/cache/trusted_config_store.py`:
- Around line 142-153: Replace the manual parent-walk in _has_symlink_component
with a simpler resolve-based check: use Path.resolve(strict=False) and return
True when path != path.resolve(strict=False) (this detects any symlink component
anywhere in the path while avoiding raising on broken symlinks); update the
function to call Path.resolve(strict=False) on the input Path and return the
comparison result instead of the loop and OSError handling.

In `@tests/test_cli.py`:
- Around line 179-205: The test function
test_scan_can_apply_local_config_once_when_confirmed has untyped parameters; add
explicit type annotations to match repo rules by importing Path from pathlib and
annotating tmp_path: Path and monkeypatch: pytest.MonkeyPatch (ensure pytest is
imported or referenced) and keep the existing return type -> None; apply the
same parameter annotation fixes to the other new/nearby test functions mentioned
in the comment to ensure all tests in this diff use tmp_path: Path and
monkeypatch: pytest.MonkeyPatch.
- Around line 179-205: The test must also assert that choosing "use once" did
not persist trust; after invoking the CLI in
test_scan_can_apply_local_config_once_when_confirmed, instantiate the same
TrustedConfigStore used in the monkeypatch (TrustedConfigStore(tmp_path /
"cache" / "trusted_local_configs.json")) or read that JSON file and assert it
either does not exist or contains no trusted entries for the model path,
ensuring the one-time branch didn't write a trusted-config record.

In `@tests/test_rules.py`:
- Around line 190-198: The test method
test_from_cli_args_uses_provided_base_config is missing the return type
annotation; update its signature to include -> None (i.e., def
test_from_cli_args_uses_provided_base_config(self) -> None:) so it follows the
repo's typing convention for tests; locate and edit the method declaration in
tests/test_rules.py to add the annotation without changing the body or
assertions.
- Around line 246-259: The test only passes a single path so it never exercises
the multi-path branch of find_local_config_for_paths; update the
test_find_local_config_for_paths_uses_shared_ancestor to create a second file
under a different subdirectory of the same repo root (e.g., create another
nested folder and file alongside nested/model.pkl), call
find_local_config_for_paths with both file paths, and assert the returned
candidate is not None and still points to the same config_dir and config_path
(root and root / ".modelaudit.toml") to validate the shared-ancestor merging
logic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6d033ff6-bb5a-4b45-8ff7-01bde0fda348

📥 Commits

Reviewing files that changed from the base of the PR and between d9fe283 and 11ec3e1.

📒 Files selected for processing (8)
  • CHANGELOG.md
  • docs/user/security-model.md
  • modelaudit/cache/trusted_config_store.py
  • modelaudit/cli.py
  • modelaudit/config/local_config.py
  • modelaudit/config/rule_config.py
  • tests/test_cli.py
  • tests/test_rules.py

Comment on lines +142 to +153
def _has_symlink_component(path: Path) -> bool:
"""Return True when path or an ancestor is a symlink."""
current = path
while True:
try:
if current.exists() and current.is_symlink():
return True
except OSError:
return True
if current == current.parent:
return False
current = current.parent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider simplifying symlink detection.

The _has_symlink_component function manually walks parent directories to detect symlinks. This is functionally correct and fail-safe (returns True on OSError).

An alternative approach would be to compare path.resolve() with the original path, which would detect symlinks anywhere in the path. However, the current implementation is more explicit and handles edge cases like broken symlinks more predictably.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@modelaudit/cache/trusted_config_store.py` around lines 142 - 153, Replace the
manual parent-walk in _has_symlink_component with a simpler resolve-based check:
use Path.resolve(strict=False) and return True when path !=
path.resolve(strict=False) (this detects any symlink component anywhere in the
path while avoiding raising on broken symlinks); update the function to call
Path.resolve(strict=False) on the input Path and return the comparison result
instead of the loop and OSError handling.

Comment on lines +246 to +259
def test_find_local_config_for_paths_uses_shared_ancestor(tmp_path: Path) -> None:
"""All scanned paths under the same config root should resolve one candidate."""
root = tmp_path / "repo"
nested = root / "models" / "nested"
nested.mkdir(parents=True)
(root / ".modelaudit.toml").write_text('suppress = ["S710"]\n')
file_path = nested / "model.pkl"
file_path.write_bytes(b"test")

candidate = find_local_config_for_paths([str(file_path)])

assert candidate is not None
assert candidate.config_dir == root
assert candidate.config_path == root / ".modelaudit.toml"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Exercise the multi-path branch in the shared-ancestor test.

With a single file, this only proves upward resolution. The equality check across multiple resolved candidates stays untested, so a regression in the actual shared-root comparison would slip through.

🧪 Cover the real shared-ancestor case
-    file_path = nested / "model.pkl"
-    file_path.write_bytes(b"test")
+    first = nested / "model-one.pkl"
+    second = nested / "model-two.pkl"
+    first.write_bytes(b"one")
+    second.write_bytes(b"two")

-    candidate = find_local_config_for_paths([str(file_path)])
+    candidate = find_local_config_for_paths([str(first), str(second)])
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/test_rules.py` around lines 246 - 259, The test only passes a single
path so it never exercises the multi-path branch of find_local_config_for_paths;
update the test_find_local_config_for_paths_uses_shared_ancestor to create a
second file under a different subdirectory of the same repo root (e.g., create
another nested folder and file alongside nested/model.pkl), call
find_local_config_for_paths with both file paths, and assert the returned
candidate is not None and still points to the same config_dir and config_path
(root and root / ".modelaudit.toml") to validate the shared-ancestor merging
logic.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@modelaudit/cache/trusted_config_store.py`:
- Around line 29-30: Add an explicit return type hint to the TrustedConfigStore
constructor: update the TrustedConfigStore.__init__ signature to annotate the
return type as -> None (e.g., def __init__(self, store_path: Path | None = None)
-> None:), leaving the body unchanged so it complies with the repo typing rule.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 55d6d1d1-50a0-4b0d-91f2-694a8ebd823b

📥 Commits

Reviewing files that changed from the base of the PR and between 11ec3e1 and 173d998.

📒 Files selected for processing (3)
  • modelaudit/cache/trusted_config_store.py
  • tests/test_cli.py
  • tests/test_rules.py

Comment on lines +29 to +30
def __init__(self, store_path: Path | None = None):
self.store_path = store_path or (Path.home() / ".modelaudit" / "cache" / "trusted_local_configs.json")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add the missing constructor return type hint.

TrustedConfigStore.__init__ should explicitly annotate -> None to comply with the repo typing rule.

♻️ Proposed fix
-class TrustedConfigStore:
+class TrustedConfigStore:
@@
-    def __init__(self, store_path: Path | None = None):
+    def __init__(self, store_path: Path | None = None) -> None:
         self.store_path = store_path or (Path.home() / ".modelaudit" / "cache" / "trusted_local_configs.json")

As per coding guidelines, **/*.py: Always include type hints in Python code.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@modelaudit/cache/trusted_config_store.py` around lines 29 - 30, Add an
explicit return type hint to the TrustedConfigStore constructor: update the
TrustedConfigStore.__init__ signature to annotate the return type as -> None
(e.g., def __init__(self, store_path: Path | None = None) -> None:), leaving the
body unchanged so it complies with the repo typing rule.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/test_cli.py`:
- Around line 208-239: Add a new test alongside
test_scan_can_remember_trusted_local_config that verifies trusted-local-config
is invalidated when the .modelaudit.toml changes: reuse TrustedConfigStore and
CliRunner, perform an initial trusted run (like calling runner.invoke(cli,
["scan", str(model_file), "--format", "text"], input="a\n") with monkeypatched
can_use_trusted_local_config and get_trusted_config_store), then mutate the
.modelaudit.toml file contents (e.g., append or modify text) before invoking cli
a second time and assert the second run no longer silently uses the trusted
config (i.e., the interactive prompt or "Found local ModelAudit config" message
appears on the second run), and also assert the trust_store.store_path still
exists but the trust decision was invalidated after the file change; reference
test_scan_can_remember_trusted_local_config, TrustedConfigStore, runner.invoke,
and .modelaudit.toml when locating where to add this test.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: fc5aa424-a75a-450e-a759-d9cab3cf152d

📥 Commits

Reviewing files that changed from the base of the PR and between 173d998 and ddaa6fc.

📒 Files selected for processing (1)
  • tests/test_cli.py

Comment on lines +208 to +239
def test_scan_can_remember_trusted_local_config(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
"""Choosing to trust a local config should persist for future interactive runs."""
import tarfile

model_file = tmp_path / "evil.tar"
with tarfile.open(model_file, "w") as tar:
payload = tmp_path / "payload.txt"
payload.write_text("content")
tar.add(payload, arcname="../evil.txt")

(tmp_path / ".modelaudit.toml").write_text('suppress = ["S405"]\n')
trust_store = TrustedConfigStore(tmp_path / "cache" / "trusted_local_configs.json")

monkeypatch.chdir(tmp_path)
monkeypatch.setattr("modelaudit.cli.can_use_trusted_local_config", lambda output_format: output_format == "text")
monkeypatch.setattr("modelaudit.cli.get_trusted_config_store", lambda: trust_store)

runner = CliRunner()
first_result = runner.invoke(
cli,
["scan", str(model_file), "--format", "text"],
input="a\n",
catch_exceptions=False,
)
second_result = runner.invoke(cli, ["scan", str(model_file), "--format", "text"], catch_exceptions=False)

assert first_result.exit_code == 0
assert second_result.exit_code == 0
assert trust_store.store_path.exists()
assert "Found local ModelAudit config" in strip_ansi(first_result.output)
assert "Found local ModelAudit config" not in strip_ansi(second_result.output)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add a regression test for trusted-config invalidation after file changes.

This verifies persistence, but not the required invalidation path when .modelaudit.toml changes. Please add a follow-up test that mutates the config between runs and asserts trust is no longer honored automatically.

♻️ Suggested test shape
+def test_scan_invalidates_trusted_local_config_when_config_changes(
+    tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    import tarfile
+
+    model_file = tmp_path / "evil.tar"
+    with tarfile.open(model_file, "w") as tar:
+        payload = tmp_path / "payload.txt"
+        payload.write_text("content")
+        tar.add(payload, arcname="../evil.txt")
+
+    config_path = tmp_path / ".modelaudit.toml"
+    config_path.write_text('suppress = ["S405"]\n')
+    trust_store = TrustedConfigStore(tmp_path / "cache" / "trusted_local_configs.json")
+
+    monkeypatch.chdir(tmp_path)
+    monkeypatch.setattr("modelaudit.cli.can_use_trusted_local_config", lambda output_format: output_format == "text")
+    monkeypatch.setattr("modelaudit.cli.get_trusted_config_store", lambda: trust_store)
+
+    runner = CliRunner()
+    first = runner.invoke(cli, ["scan", str(model_file), "--format", "text"], input="a\n", catch_exceptions=False)
+    assert first.exit_code == 0
+
+    config_path.write_text('suppress = []\n')
+
+    second = runner.invoke(cli, ["scan", str(model_file), "--format", "text"], input="n\n", catch_exceptions=False)
+    second_output = strip_ansi(second.output)
+    assert "Found local ModelAudit config" in second_output
+    assert second.exit_code == 1
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/test_cli.py` around lines 208 - 239, Add a new test alongside
test_scan_can_remember_trusted_local_config that verifies trusted-local-config
is invalidated when the .modelaudit.toml changes: reuse TrustedConfigStore and
CliRunner, perform an initial trusted run (like calling runner.invoke(cli,
["scan", str(model_file), "--format", "text"], input="a\n") with monkeypatched
can_use_trusted_local_config and get_trusted_config_store), then mutate the
.modelaudit.toml file contents (e.g., append or modify text) before invoking cli
a second time and assert the second run no longer silently uses the trusted
config (i.e., the interactive prompt or "Found local ModelAudit config" message
appears on the second run), and also assert the trust_store.store_path still
exists but the trust decision was invalidated after the file change; reference
test_scan_can_remember_trusted_local_config, TrustedConfigStore, runner.invoke,
and .modelaudit.toml when locating where to add this test.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
tests/test_cli.py (1)

210-241: ⚠️ Potential issue | 🟠 Major

Missing CLI regression for trust invalidation after config mutation.

This test verifies persistence, but not the required invalidation path. Between Line 233 and Line 234, mutate .modelaudit.toml and assert the second run re-prompts (or at least re-detects) instead of silently honoring prior trust.

♻️ Suggested test update
 def test_scan_can_remember_trusted_local_config(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
@@
-    (tmp_path / ".modelaudit.toml").write_text('suppress = ["S405"]\n')
+    config_path = tmp_path / ".modelaudit.toml"
+    config_path.write_text('suppress = ["S405"]\n')
@@
     first_result = runner.invoke(
         cli,
         ["scan", str(model_file), "--format", "text"],
         input="a\n",
         catch_exceptions=False,
     )
-    second_result = runner.invoke(cli, ["scan", str(model_file), "--format", "text"], catch_exceptions=False)
+    # Mutate config to invalidate remembered trust
+    config_path.write_text("suppress = []\n")
+    second_result = runner.invoke(
+        cli,
+        ["scan", str(model_file), "--format", "text"],
+        input="n\n",
+        catch_exceptions=False,
+    )
@@
-    assert second_result.exit_code == 0
+    assert second_result.exit_code == 1
     assert trust_store.store_path.exists()
     assert "Found local ModelAudit config" in strip_ansi(first_result.output)
-    assert "Found local ModelAudit config" not in strip_ansi(second_result.output)
+    assert "Found local ModelAudit config" in strip_ansi(second_result.output)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/test_cli.py` around lines 210 - 241, Update the
test_scan_can_remember_trusted_local_config to exercise trust invalidation:
after the first runner.invoke call (the variable first_result) mutate the local
config file (the .modelaudit.toml file in tmp_path) so its contents change, then
run the second runner.invoke; assert that the CLI (invoked via runner.invoke on
cli with "scan") no longer silently honors the prior trust by checking that the
prompt/detection for "Found local ModelAudit config" appears (or that a
re-prompt occurs) in the second_result output, and ensure the TrustedConfigStore
(trust_store) still exists but the CLI re-evaluated the config; keep existing
monkeypatch usage for can_use_trusted_local_config and get_trusted_config_store
unchanged so only the config mutation triggers invalidation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/test_cli.py`:
- Around line 257-265: The test relies on prompting behavior tied to output
format but invokes the CLI without explicitly setting format; update the
invocation of cli in the test (the runner.invoke call that uses CliRunner and
the cli function with target_file) to pass the explicit flag "--format text" in
the argument list so can_use_trusted_local_config(output_format) sees "text"
deterministically (refer to can_use_trusted_local_config,
get_trusted_config_store, scan_model_directory_or_file, CliRunner, cli, and
target_file to locate the call).

---

Duplicate comments:
In `@tests/test_cli.py`:
- Around line 210-241: Update the test_scan_can_remember_trusted_local_config to
exercise trust invalidation: after the first runner.invoke call (the variable
first_result) mutate the local config file (the .modelaudit.toml file in
tmp_path) so its contents change, then run the second runner.invoke; assert that
the CLI (invoked via runner.invoke on cli with "scan") no longer silently honors
the prior trust by checking that the prompt/detection for "Found local
ModelAudit config" appears (or that a re-prompt occurs) in the second_result
output, and ensure the TrustedConfigStore (trust_store) still exists but the CLI
re-evaluated the config; keep existing monkeypatch usage for
can_use_trusted_local_config and get_trusted_config_store unchanged so only the
config mutation triggers invalidation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 52c5d432-0709-40de-b875-1bbb3f08d9e0

📥 Commits

Reviewing files that changed from the base of the PR and between ddaa6fc and b64b9b6.

📒 Files selected for processing (1)
  • tests/test_cli.py

Comment on lines +257 to +265
monkeypatch.setattr("modelaudit.cli.can_use_trusted_local_config", lambda output_format: output_format == "text")
monkeypatch.setattr(
"modelaudit.cli.get_trusted_config_store",
lambda: TrustedConfigStore(tmp_path / "cache" / "trusted_local_configs.json"),
)
monkeypatch.setattr("modelaudit.cli.scan_model_directory_or_file", fake_scan_model_directory_or_file)

runner = CliRunner()
result = runner.invoke(cli, ["scan", str(target_file)], input="y\n", catch_exceptions=False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Make output format explicit to keep this test deterministic.

Line 257 gates trusted-config prompting to "text", but Line 265 relies on the CLI default format. Add --format text so this test remains stable if defaults change.

♻️ Suggested fix
-    result = runner.invoke(cli, ["scan", str(target_file)], input="y\n", catch_exceptions=False)
+    result = runner.invoke(
+        cli,
+        ["scan", str(target_file), "--format", "text"],
+        input="y\n",
+        catch_exceptions=False,
+    )

As per coding guidelines, tests/**/*.py: “Use deterministic fixtures only in tests …”.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/test_cli.py` around lines 257 - 265, The test relies on prompting
behavior tied to output format but invokes the CLI without explicitly setting
format; update the invocation of cli in the test (the runner.invoke call that
uses CliRunner and the cli function with target_file) to pass the explicit flag
"--format text" in the argument list so
can_use_trusted_local_config(output_format) sees "text" deterministically (refer
to can_use_trusted_local_config, get_trusted_config_store,
scan_model_directory_or_file, CliRunner, cli, and target_file to locate the
call).

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@CHANGELOG.md`:
- Line 84: The changelog line uses the phrase "interactive text run," which is
unclear; update the sentence in CHANGELOG.md replacing "interactive text run"
with a clearer term such as "interactive scan" or "interactive mode" (e.g.,
change the bullet text under **security:** to read "...unless a human explicitly
trusts that config in an interactive scan; remembered trust..."), keeping the
rest of the sentence intact and preserving the note about remembered trust and
invalidation when the config changes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3bdba673-11d0-48ee-95b0-bcd8481aed52

📥 Commits

Reviewing files that changed from the base of the PR and between b64b9b6 and a0878c7.

📒 Files selected for processing (1)
  • CHANGELOG.md


### Fixed

- **security:** stop auto-applying local `.modelaudit.toml` and `pyproject.toml` rule config during scans unless a human explicitly trusts that config in an interactive text run; remembered trust is stored securely under the local ModelAudit cache and invalidated when the config changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Optional: Consider clarifying "interactive text run" terminology.

The entry accurately describes the security fix. However, the phrase "interactive text run" could be more idiomatic. Consider alternatives like "interactive scan", "interactive mode", or "interactive session" for better clarity.

Example:

-- **security:** stop auto-applying local `.modelaudit.toml` and `pyproject.toml` rule config during scans unless a human explicitly trusts that config in an interactive text run; remembered trust is stored securely under the local ModelAudit cache and invalidated when the config changes
+- **security:** stop auto-applying local `.modelaudit.toml` and `pyproject.toml` rule config during scans unless a human explicitly trusts that config in an interactive scan; remembered trust is stored securely under the local ModelAudit cache and invalidated when the config changes
📝 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
- **security:** stop auto-applying local `.modelaudit.toml` and `pyproject.toml` rule config during scans unless a human explicitly trusts that config in an interactive text run; remembered trust is stored securely under the local ModelAudit cache and invalidated when the config changes
- **security:** stop auto-applying local `.modelaudit.toml` and `pyproject.toml` rule config during scans unless a human explicitly trusts that config in an interactive scan; remembered trust is stored securely under the local ModelAudit cache and invalidated when the config changes
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@CHANGELOG.md` at line 84, The changelog line uses the phrase "interactive
text run," which is unclear; update the sentence in CHANGELOG.md replacing
"interactive text run" with a clearer term such as "interactive scan" or
"interactive mode" (e.g., change the bullet text under **security:** to read
"...unless a human explicitly trusts that config in an interactive scan;
remembered trust..."), keeping the rest of the sentence intact and preserving
the note about remembered trust and invalidation when the config changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant