microsoft / microsoft/agent-governance-toolkit

ScopeGuard.evaluate_from_git returns PASS when the diff cannot be measured (wrong base branch, missing git, non-repo path)

Open
#3,505 3 comments 0 reactions 0 assignees View on GitHub
needs-review:HIGH
Dominant language
Python
Stars
6.3k
Forks
1.1k
Avg merge
5d 11h
Merged PRs (30d)
142

Description

### Description

`ScopeGuard.evaluate_from_git` returns `PASS` for a change of any size whenever the diff cannot be measured.

`_get_diff_stats` (`scope_guard.py:85-120`) reports every failure as `([], 0, 0)`. The limit checks in `evaluate` read that as a zero-file, zero-line change -- the most in-scope change possible -- so the guard reports `decision="PASS"`, `reason="All scope checks passed"`.

### Reproduction

A 30-file, 15000-line staged change in a repo whose default branch is not `main`:

```python
from agent_os.integrations.scope_guard import ScopeGuard, ScopeConfig

cfg = ScopeConfig(max_files=10, max_lines=500)
guard = ScopeGuard()

# repo is on "trunk"; base_branch defaults to "main"
ev = guard.evaluate_from_git("agent-1", cfg, repo_with_30_changed_files, "main")
print(ev.decision, "|", ev.reason)
```

```
PASS | All scope checks passed
```

With the correct base branch the same change is correctly refused:

```
HARD_FAIL | files changed (30) exceeds 2x limit (20); lines changed (15000) exceeds 2x limit (1000)
```

### Failure modes

| Case | `git diff` result | Current | Expected |
|---|---|---|---|
| Base branch not present | exits 128 | `PASS` | block |
| `repo_path` is not a repository | exits 129 | `PASS` | block |
| `git` not installed | `FileNotFoundError` | `PASS` | block |
| `git diff` times out | `TimeoutExpired` | `PASS` | block |
| Unparseable numstat row | row skipped | totals lowered | block |
| `repo_path` does not exist | `NotADirectoryError` | **raises `OSError`** | block |

The first row is the likeliest in practice: `base_branch` defaults to `"main"`, so a repository on `master`/`trunk`, or a shallow CI clone that never fetched the base, silently turns the guard off. The last row does not fail open but is also wrong -- `NotADirectoryError` and `PermissionError` are not caught, so a bad `repo_path` propagates out of a governance check as a raw `OSError`.

### Root cause

Two things collapse into one value:

```python
except (subprocess.TimeoutExpired, FileNotFoundError) as exc:
logger.warning("_get_diff_stats failed: %s", exc)
return [], 0, 0
```

"the diff is empty" and "we could not read the diff" both produce `([], 0, 0)`. `returncode` is never checked at all -- and since `git diff` without `--exit-code` returns 0 whether or not there are differences, a nonzero status is unambiguously a failure. The `len(parts) == 3` filter silently drops rows it cannot parse, which under-measures the totals the limits are compared against for the same reason.

### Proposed fix

Have `_get_diff_stats` return a fourth element that is `None` on success and describes the failure otherwise; check `returncode`; catch `OSError` rather than `FileNotFoundError`; treat an unparseable row as an error. `evaluate_from_git` then returns `HARD_FAIL` with a new `ScopeEvaluation.error` field, and records it to the policy engine so the block is auditable.

An unmeasured change is not a change within scope. Two behaviours are deliberately preserved:

- `mode == "off"` still returns `PASS` -- the operator has opted out of the check, so there is nothing to fail closed on.
- A genuinely empty diff still returns `PASS`. This is the other half of the fix, since it previously produced the same `([], 0, 0)` as a failure.

`evaluate` itself is unchanged, so callers that measure the diff themselves keep their current behaviour.

### Scope note

`ScopeGuard` is registered as a detection module in `BaseIntegration` (`base.py:1273-1282`) but `_check_detection_modules` has no `scope_guard` branch, so nothing in AGT currently calls `evaluate_from_git` -- consistent with what `docs/compliance/owasp-llm-top10-mapping.md:374` already records ("Returns advisory `ScopeEvaluation`; no production code checks the decision"). So this is not an active bypass in the shipped enforcement path; it is a fail-open default in a published governance API that external callers do use, and one that would become an active bypass the moment the enforcement wiring that doc plans is added.

No cross-SDK twin exists -- `ScopeGuard` is Python-only (no `numstat` or `scope_guard` equivalent in the Rust, Go, .NET or TypeScript trees), so there is no parity change to make.

### Environment

- `agent-governance-python/agent-os`, `main` @ 24d5725
- Python 3.12, Windows

I have a fix with 15 regression tests ready to open as a PR.

Contributor guide

Open the contributing guide

Research direction

Start in agent-os/.../scope_guard.py, especially _get_diff_stats at lines 85-120 and evaluate_from_git; inspect ScopeEvaluation and the policy-engine recording path. Run the 15 regression tests described in the issue. Done means diff failures and unparseable rows produce an auditable HARD_FAIL, while empty diffs and mode off retain PASS.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.