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)
- 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
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