From 9414494b39ea2b13a334e6acaaf1c6482d94d781 Mon Sep 17 00:00:00 2001 From: Charles Vien Date: Sat, 29 Aug 2026 23:31:16 -0700 Subject: [PATCH 1/5] Make stamphog line ceiling delegable per folder --- .stamphog/README.md | 12 +- .stamphog/policy.yml | 2 + .../backend/logic/policy_defaults/policy.yml | 2 + .../packages/pr-approval-agent/README.md | 2 + .../packages/pr-approval-agent/policy.py | 118 +++++++++++------- .../packages/pr-approval-agent/review_pr.py | 22 ++-- .../packages/pr-approval-agent/test_policy.py | 98 +++++++++++---- 7 files changed, 175 insertions(+), 81 deletions(-) diff --git a/.stamphog/README.md b/.stamphog/README.md index 5e9fc8798c3b..f3c109afb28e 100644 --- a/.stamphog/README.md +++ b/.stamphog/README.md @@ -21,19 +21,19 @@ A folder may carry an `AGENT_APPROVALS.md` with a `stamphog:` frontmatter block Resolution: - Every `AGENT_APPROVALS.md` at or above a changed file governs it: guidance accumulates outermost first, and a child file adds to its ancestors rather than replacing them. -- For the delegated `size_gate.max_files`, the nearest file on the chain with a valid grant wins for its files (within the contract ceiling); files whose chain grants nothing belong to the global pool. +- For the delegated `size_gate.max_files` and `size_gate.max_lines`, the nearest file on the chain with a valid grant wins for its files (within the contract ceilings). A file that grants only one key inherits the other from the nearest ancestor grant, or from the global value when no ancestor grants it. Files whose chain grants nothing belong to the global pool. - The frontmatter is a positive allow-list: only keys named in the `overrides` contract in `policy.yml` are read, within their ceilings. Anything else (unknown key, out-of-bounds value, unparseable frontmatter) invalidates the whole file - frontmatter and prose. An invalid file contributes nothing itself, but it does not cancel its ancestors: files under it still ride an ancestor's grant, or fall to the global pool if the chain grants nothing. Rationale: an author who can write an invalid file could equally delete it, so treating invalid as absent grants no extra power, and every `AGENT_APPROVALS.md` edit is human-reviewed via the `stamphog_policy` deny anyway. - The prose is untrusted advisory guidance. It is sanitized, length-capped, and injected inside the reviewer prompt's untrusted region; it can never override the deny rules or the refusal criteria. ### Mixed PRs get mixed leniency -Each scope's files are counted against that scope's own file ceiling, so a grant covers exactly the files that resolve to it (the nearest valid grant on their chain) and nothing else. +Each scope's files are counted against that scope's own line and file ceilings, so a grant covers exactly the files that resolve to it (the nearest valid grant on their chain) and nothing else. Example: a PR changing 30 files under `products/visual_review/` (ceiling 50) plus 19 files elsewhere (global ceiling 20) passes, because each budget fits. Add a 21st global file and the PR is denied for the global budget, no matter how much headroom the folder still has. -Files whose chain grants no valid `max_files` (no folder file, prose-only, or only invalid grants) count against the global budget, so splitting files across pseudo-scopes can never inflate the allowance. -The line ceiling stays a single global total; it is not delegable. +Files whose chain grants nothing (no folder file, prose-only, or only invalid grants) count against the global budget, so splitting files across pseudo-scopes can never inflate the allowance. +Lines follow the same rule: a scope's substantive lines are counted against that scope's own line ceiling, and the global pool's lines against the global line ceiling. ## Delegation contract -The set of keys a folder file may override lives under `overrides` in `policy.yml` (currently just `size_gate.max_files`, ceiling 50). -deny, allow, dismiss, tiers, and `size_gate.max_lines` are non-delegable by construction - they are absent from the contract and cannot be granted from a folder file. +The set of keys a folder file may override lives under `overrides` in `policy.yml` (currently `size_gate.max_files`, ceiling 50, and `size_gate.max_lines`, ceiling 1000). +deny, allow, dismiss, and tiers are non-delegable by construction - they are absent from the contract and cannot be granted from a folder file. diff --git a/.stamphog/policy.yml b/.stamphog/policy.yml index ce32dd7c9dcc..33a9d13b2430 100644 --- a/.stamphog/policy.yml +++ b/.stamphog/policy.yml @@ -185,6 +185,8 @@ tiers: overrides: 'size_gate.max_files': ceiling: 50 + 'size_gate.max_lines': + ceiling: 1000 # Author-familiarity signal - judgment layer only, never a gate. Non-delegable # (absent from `overrides`). STRONG = blame overlap ≥ min_blame_overlap_pct; # MODERATE = both of its keys satisfied; else NONE. diff --git a/products/stamphog/backend/logic/policy_defaults/policy.yml b/products/stamphog/backend/logic/policy_defaults/policy.yml index 0163ae4fe67d..4660a63b6f6d 100644 --- a/products/stamphog/backend/logic/policy_defaults/policy.yml +++ b/products/stamphog/backend/logic/policy_defaults/policy.yml @@ -180,6 +180,8 @@ tiers: overrides: 'size_gate.max_files': ceiling: 50 + 'size_gate.max_lines': + ceiling: 1000 # Author-familiarity signal — judgment layer only, never a gate. Non-delegable (absent from # `overrides`). STRONG = blame overlap >= min_blame_overlap_pct; MODERATE = both of its keys satisfied; # else NONE. Thresholds are stamphog's backtested defaults. diff --git a/products/stamphog/packages/pr-approval-agent/README.md b/products/stamphog/packages/pr-approval-agent/README.md index ca05de12bef1..9c6916083f0b 100644 --- a/products/stamphog/packages/pr-approval-agent/README.md +++ b/products/stamphog/packages/pr-approval-agent/README.md @@ -71,6 +71,8 @@ Size ceiling (hard gate) (limits derived from 90 days of denial outcomes: the friction cluster of denied-yet-merged-unchanged PRs sits at 500-750 substantive lines, and past ~800 the merged-unchanged rate collapses, so escalation is genuinely right) + - A folder's AGENT_APPROVALS.md can raise either ceiling for its own files, + within the `overrides` contract in policy.yml (see .stamphog/README.md) - Docs (.md/.txt/.rst anywhere; artifact-extension files under docs/), snapshots (.snap/.ambr, __snapshots__/), images, `.lock`-extension files (e.g. `yarn.lock`), tests (test dirs and diff --git a/products/stamphog/packages/pr-approval-agent/policy.py b/products/stamphog/packages/pr-approval-agent/policy.py index da69c0749819..56c97cec078a 100644 --- a/products/stamphog/packages/pr-approval-agent/policy.py +++ b/products/stamphog/packages/pr-approval-agent/policy.py @@ -184,13 +184,16 @@ def deny_pattern_defs(self) -> dict[str, dict[str, list[str]]]: class ScopeBudget: """One size-gate budget: a folder override's files, or the global pool. - `path` is the granting AGENT_APPROVALS.md (repo-relative); None is the global - pool, which absorbs every file whose chain grants no valid max_files so - splitting files across pseudo-scopes can never inflate the allowance. + `path` is the nearest granting AGENT_APPROVALS.md (repo-relative); None is + the global pool, which absorbs every file whose chain grants nothing so + splitting files across pseudo-scopes can never inflate the allowance. Each + ceiling comes from the nearest grant of that key on the chain, falling back + to the global value when no folder on the chain grants it. """ path: str | None max_files: int + max_lines: int files: tuple[str, ...] @@ -200,13 +203,12 @@ class EffectivePolicy: Mixed PRs get mixed leniency: every AGENT_APPROVALS.md at or above a changed file governs it, and the file is budgeted by the nearest folder on that - chain with a valid max_files grant. Each scope's files must fit that scope's - own ceiling; files with no valid grant on their chain keep the global - ceiling. No file ever gets more leniency than its own chain grants. - max_lines stays a single global total; it is not delegable. + chain with a valid size_gate grant. Each scope's files must fit that scope's + own line and file ceilings; files with no valid grant on their chain keep + the global ceilings. No file ever gets more leniency than its own chain + grants. """ - max_lines: int scopes: tuple[ScopeBudget, ...] folder_prose: str | None = None invalid_folder_files: tuple[str, ...] = () @@ -229,9 +231,10 @@ class PolicyError(ValueError): _DENY_SCOPES = {"any", "titles", "paths"} _BREADTH_RULES = {"single-area", "not-cross-cutting"} -# The delegation contract's only delegable key. Everything else (deny, allow, -# tiers, size_gate.max_lines) is non-delegable by construction. -_DELEGABLE_KEYS = {"size_gate.max_files"} +# The delegation contract's delegable keys. Everything else (deny, allow, tiers) +# is non-delegable by construction. +_DELEGABLE_KEYS = {"size_gate.max_files", "size_gate.max_lines"} +_SIZE_GATE_DELEGATIONS = {"max_files": "size_gate.max_files", "max_lines": "size_gate.max_lines"} # Invariant 7: self-governance deny must cover these path families so a future # policy edit cannot silently drop stamphog's protection of its own files. @@ -505,9 +508,14 @@ class _FolderOverride: """Result of parsing a folder AGENT_APPROVALS.md.""" max_files: int | None = None + max_lines: int | None = None prose: str | None = None invalid: bool = False + @property + def grants(self) -> bool: + return self.max_files is not None or self.max_lines is not None + def _scope_chain_for( file_path: str, root: Path, cache: dict[PurePosixPath, tuple[PurePosixPath, ...]] @@ -579,42 +587,46 @@ def _parse_folder_policy(path: Path, contract: dict[str, OverrideContract]) -> _ stamphog = frontmatter.get("stamphog") if stamphog is None: # Advisory-only file: no delegated override, prose still applies. - return _FolderOverride(max_files=None, prose=prose or None) + return _FolderOverride(prose=prose or None) if not isinstance(stamphog, dict): return _FolderOverride(invalid=True) - # Positive allow-list: the only delegated path is size_gate.max_files. - max_files = _read_delegated_max_files(stamphog, contract) - if max_files is None: + # Positive allow-list: the only delegated paths are the size_gate ceilings. + grants = _read_delegated_size_gate(stamphog, contract) + if grants is None: return _FolderOverride(invalid=True) - return _FolderOverride(max_files=max_files, prose=prose or None) + return _FolderOverride(max_files=grants.get("max_files"), max_lines=grants.get("max_lines"), prose=prose or None) -def _read_delegated_max_files(stamphog: dict[str, Any], contract: dict[str, OverrideContract]) -> int | None: - """Return the delegated max_files if valid and within ceiling, else None (invalid).""" - if "size_gate.max_files" not in contract: - return None +def _read_delegated_size_gate(stamphog: dict[str, Any], contract: dict[str, OverrideContract]) -> dict[str, int] | None: + """Return the delegated size_gate grants if every key is delegated and within its ceiling, else None (invalid).""" if set(stamphog) - {"size_gate"}: return None size_gate = stamphog.get("size_gate") - if not isinstance(size_gate, dict) or set(size_gate) - {"max_files"}: - return None - value = size_gate.get("max_files") - if not isinstance(value, int) or isinstance(value, bool): - return None - ceiling = contract["size_gate.max_files"].ceiling - if value < 1 or value > ceiling: + if not isinstance(size_gate, dict) or not size_gate: return None - return value + grants: dict[str, int] = {} + for key, value in size_gate.items(): + contract_key = _SIZE_GATE_DELEGATIONS.get(key) if isinstance(key, str) else None + if contract_key is None or contract_key not in contract: + return None + if not isinstance(value, int) or isinstance(value, bool): + return None + if value < 1 or value > contract[contract_key].ceiling: + return None + grants[key] = value + return grants def resolve(policy: Policy, changed_files: list[str]) -> EffectivePolicy: """Resolve the per-scope size budgets for a PR's changed files. Every AGENT_APPROVALS.md at or above a changed file governs it. A file's size - budget comes from the nearest folder on its chain with a valid max_files - grant; files whose chain grants nothing (no folder file, prose-only, or only - invalid grants) pool into the global budget. Advisory prose accumulates from + budget comes from the nearest folder on its chain with a valid size_gate + grant, and each ceiling in that budget is the nearest grant of that key on + the chain (a child granting only max_lines still rides its parent's + max_files); files whose chain grants nothing (no folder file, prose-only, or + only invalid grants) pool into the global budget. Advisory prose accumulates from every valid folder file on the chain of at least one changed file, outermost first so general guidance precedes specific. An invalid folder file is treated as absent - it grants nothing and adds no prose, but its ancestors @@ -633,24 +645,40 @@ def parsed_for(scope_dir: PurePosixPath) -> tuple[str, _FolderOverride]: # Files sharing a granting AGENT_APPROVALS.md pool into one budget; the folder # files touched by any chain feed the prose and invalid-file reporting. grant_files: dict[str, list[str]] = {} - grant_max: dict[str, int] = {} + grant_budget: dict[str, tuple[int, int]] = {} # rel path -> (max_files, max_lines) global_files: list[str] = [] on_chain: dict[str, _FolderOverride] = {} # rel path -> parse, each file once for file_path in changed_files: - grant: tuple[str, int] | None = None + scope_path: str | None = None + max_files: int | None = None + max_lines: int | None = None for scope_dir in _scope_chain_for(file_path, root, dir_cache): rel_path, parsed = parsed_for(scope_dir) on_chain[rel_path] = parsed - if grant is None and not parsed.invalid and parsed.max_files is not None: - grant = (rel_path, parsed.max_files) - if grant is None: + if parsed.invalid or not parsed.grants: + continue + if scope_path is None: + scope_path = rel_path + if max_files is None: + max_files = parsed.max_files + if max_lines is None: + max_lines = parsed.max_lines + if scope_path is None: global_files.append(file_path) - else: - grant_files.setdefault(grant[0], []).append(file_path) - grant_max[grant[0]] = grant[1] + continue + grant_files.setdefault(scope_path, []).append(file_path) + grant_budget[scope_path] = ( + policy.size_gate.max_files if max_files is None else max_files, + policy.size_gate.max_lines if max_lines is None else max_lines, + ) override_scopes = [ - ScopeBudget(path=rel_path, max_files=grant_max[rel_path], files=tuple(files)) + ScopeBudget( + path=rel_path, + max_files=grant_budget[rel_path][0], + max_lines=grant_budget[rel_path][1], + files=tuple(files), + ) for rel_path, files in sorted(grant_files.items()) ] @@ -672,10 +700,14 @@ def parsed_for(scope_dir: PurePosixPath) -> tuple[str, _FolderOverride]: else: folder_prose = None - scopes = (*override_scopes, ScopeBudget(path=None, max_files=policy.size_gate.max_files, files=tuple(global_files))) - return EffectivePolicy( + global_scope = ScopeBudget( + path=None, + max_files=policy.size_gate.max_files, max_lines=policy.size_gate.max_lines, - scopes=scopes, + files=tuple(global_files), + ) + return EffectivePolicy( + scopes=(*override_scopes, global_scope), folder_prose=folder_prose, invalid_folder_files=tuple(invalid_files), ) diff --git a/products/stamphog/packages/pr-approval-agent/review_pr.py b/products/stamphog/packages/pr-approval-agent/review_pr.py index 59b8a6d37042..eda6f3f8f22b 100644 --- a/products/stamphog/packages/pr-approval-agent/review_pr.py +++ b/products/stamphog/packages/pr-approval-agent/review_pr.py @@ -672,7 +672,6 @@ def _summarize_ownership(self) -> str: def _check_size(self) -> tuple[bool, str]: lines, files = substantive_size(self.pr.files) - max_lines = self.effective_policy.max_lines if self.effective_policy else MAX_LINES binary_count = sum(1 for f in self.pr.files if f.get("binary")) exempt_files = len(self.pr.files) - files suffix_parts = [] @@ -681,19 +680,20 @@ def _check_size(self) -> tuple[bool, str]: if exempt_files: suffix_parts.append(f"{self.pr.lines_total}L/{len(self.pr.files)}F incl. docs/generated/snapshots") suffix = (", " + "; ".join(suffix_parts)) if suffix_parts else "" - if lines > max_lines: - return ( - False, - f"too large for auto-review ({lines}L, {files}F substantive{suffix} — ceiling is {max_lines}L)", - ) # Mixed PRs get mixed leniency: each file counts against the budget of # the scope governing it (a folder override or the global pool), so a - # folder's higher ceiling covers its own files and nothing else. + # folder's higher ceilings cover its own files and nothing else. for scope in self._size_scopes(): in_scope = set(scope.files) - _, scope_files = substantive_size([f for f in self.pr.files if f["filename"] in in_scope]) + scope_lines, scope_files = substantive_size([f for f in self.pr.files if f["filename"] in in_scope]) + where = scope.path or "global" + if scope_lines > scope.max_lines: + return ( + False, + f"too large for auto-review ({scope_lines}L substantive in {where} — " + f"ceiling is {scope.max_lines}L; {lines}L, {files}F total{suffix})", + ) if scope_files > scope.max_files: - where = scope.path or "global" return ( False, f"too large for auto-review ({scope_files}F substantive in {where} — " @@ -705,7 +705,7 @@ def _size_scopes(self) -> tuple[ScopeBudget, ...]: if self.effective_policy is not None: return self.effective_policy.scopes all_files = tuple(f["filename"] for f in self.pr.files) - return (ScopeBudget(path=None, max_files=MAX_FILES, files=all_files),) + return (ScopeBudget(path=None, max_files=MAX_FILES, max_lines=MAX_LINES, files=all_files),) def _check_tier(self) -> tuple[bool, str]: cl = self.classification @@ -1081,7 +1081,7 @@ def to_dict(self) -> dict: "policy_file": ".stamphog/policy.yml", "scopes": ( [ - {"path": s.path, "max_files": s.max_files, "files": len(s.files)} + {"path": s.path, "max_files": s.max_files, "max_lines": s.max_lines, "files": len(s.files)} for s in self.effective_policy.scopes ] if self.effective_policy diff --git a/products/stamphog/packages/pr-approval-agent/test_policy.py b/products/stamphog/packages/pr-approval-agent/test_policy.py index 21727bb25d2a..9a3ec45a034b 100644 --- a/products/stamphog/packages/pr-approval-agent/test_policy.py +++ b/products/stamphog/packages/pr-approval-agent/test_policy.py @@ -283,8 +283,13 @@ def test_server_owned_digest_section_is_allowed_and_ignored(tmp_path: Path) -> N _PROSE_ONLY_FM = "{}" -def _grant(max_files: int) -> str: - return f"stamphog:\n size_gate:\n max_files: {max_files}" +def _grant(max_files: int | None = None, max_lines: int | None = None) -> str: + lines = ["stamphog:", " size_gate:"] + if max_files is not None: + lines.append(f" max_files: {max_files}") + if max_lines is not None: + lines.append(f" max_lines: {max_lines}") + return "\n".join(lines) def _multi_prose(*parts: tuple[str, str]) -> str: @@ -319,7 +324,7 @@ def test_resolve_folder_override_budgets_its_own_files(fake_repo: Path) -> None: assert vr.max_files == 50 assert set(vr.files) == {"products/visual_review/a.py", "products/visual_review/sub/b.py"} assert _scope(eff, None).files == () - assert eff.max_lines == gates.MAX_LINES + assert vr.max_lines == gates.MAX_LINES assert eff.invalid_folder_files == () assert eff.folder_prose == "advisory prose" @@ -336,11 +341,35 @@ def test_resolve_mixed_pr_budgets_each_scope_separately(fake_repo: Path) -> None assert _scope(eff, None).files == ("README.md",) +@pytest.mark.parametrize( + "frontmatter, max_files, max_lines", + [ + pytest.param(_grant(max_lines=1000), gates.MAX_FILES, 1000, id="lines-only"), + pytest.param(_grant(max_files=50, max_lines=1000), 50, 1000, id="both"), + ], +) +def test_resolve_folder_grant_keeps_global_ceiling_for_ungranted_key( + fake_repo: Path, frontmatter: str, max_files: int, max_lines: int +) -> None: + _write_folder_policy(fake_repo, frontmatter) + eff = resolve(gates.POLICY, ["products/visual_review/a.py", "README.md"]) + vr = _scope(eff, _VISUAL_REVIEW_FILE) + assert (vr.max_files, vr.max_lines) == (max_files, max_lines) + assert vr.files == ("products/visual_review/a.py",) + global_scope = _scope(eff, None) + assert (global_scope.max_files, global_scope.max_lines) == (gates.MAX_FILES, gates.MAX_LINES) + assert global_scope.files == ("README.md",) + + @pytest.mark.parametrize( "frontmatter", [ - pytest.param("stamphog:\n size_gate:\n max_lines: 999", id="undelegated-key"), - pytest.param("stamphog:\n size_gate:\n max_files: 99", id="over-ceiling"), + pytest.param("stamphog:\n tiers:\n max_files: 10", id="undelegated-key"), + pytest.param("stamphog:\n size_gate:\n breadth: single-area", id="undelegated-size-gate-key"), + pytest.param("stamphog:\n size_gate: {}", id="empty-grant"), + pytest.param(_grant(max_files=99), id="files-over-ceiling"), + pytest.param(_grant(max_lines=1001), id="lines-over-ceiling"), + pytest.param(_grant(max_files=50, max_lines=1001), id="one-key-over-ceiling"), ], ) def test_resolve_invalid_folder_file_pools_files_into_global(fake_repo: Path, frontmatter: str) -> None: @@ -378,17 +407,26 @@ def test_resolve_carries_sanitized_prose(fake_repo: Path) -> None: @pytest.mark.parametrize( - "n_global, expected_ok", + "vr_additions, global_additions, n_global, expected_ok, expected_where", [ - pytest.param(19, True, id="both-budgets-fit"), - pytest.param(21, False, id="global-budget-exceeded"), + pytest.param(5, 5, 19, True, None, id="both-budgets-fit"), + pytest.param(5, 5, 21, False, "global", id="global-file-budget-exceeded"), + pytest.param(5, 30, 19, False, "global", id="global-line-budget-exceeded"), + pytest.param(30, 5, 19, True, None, id="folder-lines-exceed-global-ceiling-but-fit-own"), + pytest.param(40, 5, 19, False, _VISUAL_REVIEW_FILE, id="folder-line-budget-exceeded"), ], ) -def test_size_gate_applies_mixed_leniency(n_global: int, expected_ok: bool) -> None: - # 30 folder-scoped files ride the folder's ceiling while the remaining - # files are judged against the global ceiling on their own. - vr_files = [{"filename": f"products/visual_review/f{i}.py", "additions": 5, "deletions": 0} for i in range(30)] - global_files = [{"filename": f"posthog/api/m{i}.py", "additions": 5, "deletions": 0} for i in range(n_global)] +def test_size_gate_applies_mixed_leniency( + vr_additions: int, global_additions: int, n_global: int, expected_ok: bool, expected_where: str | None +) -> None: + # 30 folder-scoped files ride the folder's ceilings while the remaining + # files are judged against the global ceilings on their own. + vr_files = [ + {"filename": f"products/visual_review/f{i}.py", "additions": vr_additions, "deletions": 0} for i in range(30) + ] + global_files = [ + {"filename": f"posthog/api/m{i}.py", "additions": global_additions, "deletions": 0} for i in range(n_global) + ] pipeline = review_pr.Pipeline(pr_number=1, repo="PostHog/posthog") pipeline.pr = PRData( @@ -409,17 +447,21 @@ def test_size_gate_applies_mixed_leniency(n_global: int, expected_ok: bool) -> N check_runs=[], ) pipeline.effective_policy = EffectivePolicy( - max_lines=500, scopes=( - ScopeBudget(path=_VISUAL_REVIEW_FILE, max_files=50, files=tuple(f["filename"] for f in vr_files)), - ScopeBudget(path=None, max_files=20, files=tuple(f["filename"] for f in global_files)), + ScopeBudget( + path=_VISUAL_REVIEW_FILE, + max_files=50, + max_lines=1000, + files=tuple(f["filename"] for f in vr_files), + ), + ScopeBudget(path=None, max_files=20, max_lines=500, files=tuple(f["filename"] for f in global_files)), ), ) ok, message = pipeline._check_size() assert ok is expected_ok - if not expected_ok: - assert "global" in message + if expected_where is not None: + assert f"in {expected_where}" in message @pytest.mark.parametrize( @@ -460,6 +502,21 @@ def test_resolve_nearest_grant_wins_across_siblings(fake_repo: Path) -> None: assert _scope(eff, None).files == () +def test_resolve_child_inherits_ungranted_key_from_parent(fake_repo: Path) -> None: + # A child granting only max_lines refines its parent's max_files grant: its + # files ride the nearest grant of each key on the chain. + _write_agent_policy(fake_repo, "products", _grant(max_files=50), "parent guidance") + _write_agent_policy(fake_repo, "products/visual_review", _grant(max_lines=1000), "child guidance") + eff = resolve(gates.POLICY, ["products/visual_review/a.py", "products/foo.py"]) + child = _scope(eff, _VISUAL_REVIEW_FILE) + assert (child.max_files, child.max_lines) == (50, 1000) + assert child.files == ("products/visual_review/a.py",) + parent = _scope(eff, _PRODUCTS_FILE) + assert (parent.max_files, parent.max_lines) == (50, gates.MAX_LINES) + assert parent.files == ("products/foo.py",) + assert _scope(eff, None).files == () + + def test_resolve_invalid_child_rides_parent_grant(fake_repo: Path) -> None: # An invalid child is treated as absent: it grants nothing and adds no prose, # but it does not cancel the granting parent above it. @@ -554,10 +611,9 @@ def _body_pipeline(fam) -> "review_pr.Pipeline": "assurance": {"head_approvals": [], "head_commented_users": ["greptile-apps[bot]"]}, } pipeline.effective_policy = EffectivePolicy( - max_lines=500, scopes=( - ScopeBudget(path=_VISUAL_REVIEW_FILE, max_files=50, files=("products/visual_review/a.py",)), - ScopeBudget(path=None, max_files=20, files=()), + ScopeBudget(path=_VISUAL_REVIEW_FILE, max_files=50, max_lines=1000, files=("products/visual_review/a.py",)), + ScopeBudget(path=None, max_files=20, max_lines=500, files=()), ), ) pipeline.gate_results = [review_pr.GateResult("size", True, "4L, 1F substantive")] From f25914979f55734d946a5f1333b69b0f0b712595 Mon Sep 17 00:00:00 2001 From: Charles Vien Date: Sat, 29 Aug 2026 23:37:37 -0700 Subject: [PATCH 2/5] Strip added comments from stamphog size gate --- .../packages/pr-approval-agent/policy.py | 21 +++++++------------ .../packages/pr-approval-agent/test_policy.py | 2 -- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/products/stamphog/packages/pr-approval-agent/policy.py b/products/stamphog/packages/pr-approval-agent/policy.py index 56c97cec078a..fcfd0fa6bdb5 100644 --- a/products/stamphog/packages/pr-approval-agent/policy.py +++ b/products/stamphog/packages/pr-approval-agent/policy.py @@ -186,9 +186,7 @@ class ScopeBudget: `path` is the nearest granting AGENT_APPROVALS.md (repo-relative); None is the global pool, which absorbs every file whose chain grants nothing so - splitting files across pseudo-scopes can never inflate the allowance. Each - ceiling comes from the nearest grant of that key on the chain, falling back - to the global value when no folder on the chain grants it. + splitting files across pseudo-scopes can never inflate the allowance. """ path: str | None @@ -599,7 +597,6 @@ def _parse_folder_policy(path: Path, contract: dict[str, OverrideContract]) -> _ def _read_delegated_size_gate(stamphog: dict[str, Any], contract: dict[str, OverrideContract]) -> dict[str, int] | None: - """Return the delegated size_gate grants if every key is delegated and within its ceiling, else None (invalid).""" if set(stamphog) - {"size_gate"}: return None size_gate = stamphog.get("size_gate") @@ -624,8 +621,7 @@ def resolve(policy: Policy, changed_files: list[str]) -> EffectivePolicy: Every AGENT_APPROVALS.md at or above a changed file governs it. A file's size budget comes from the nearest folder on its chain with a valid size_gate grant, and each ceiling in that budget is the nearest grant of that key on - the chain (a child granting only max_lines still rides its parent's - max_files); files whose chain grants nothing (no folder file, prose-only, or + the chain; files whose chain grants nothing (no folder file, prose-only, or only invalid grants) pool into the global budget. Advisory prose accumulates from every valid folder file on the chain of at least one changed file, outermost first so general guidance precedes specific. An invalid folder file is @@ -645,7 +641,8 @@ def parsed_for(scope_dir: PurePosixPath) -> tuple[str, _FolderOverride]: # Files sharing a granting AGENT_APPROVALS.md pool into one budget; the folder # files touched by any chain feed the prose and invalid-file reporting. grant_files: dict[str, list[str]] = {} - grant_budget: dict[str, tuple[int, int]] = {} # rel path -> (max_files, max_lines) + grant_max_files: dict[str, int] = {} + grant_max_lines: dict[str, int] = {} global_files: list[str] = [] on_chain: dict[str, _FolderOverride] = {} # rel path -> parse, each file once for file_path in changed_files: @@ -667,16 +664,14 @@ def parsed_for(scope_dir: PurePosixPath) -> tuple[str, _FolderOverride]: global_files.append(file_path) continue grant_files.setdefault(scope_path, []).append(file_path) - grant_budget[scope_path] = ( - policy.size_gate.max_files if max_files is None else max_files, - policy.size_gate.max_lines if max_lines is None else max_lines, - ) + grant_max_files[scope_path] = policy.size_gate.max_files if max_files is None else max_files + grant_max_lines[scope_path] = policy.size_gate.max_lines if max_lines is None else max_lines override_scopes = [ ScopeBudget( path=rel_path, - max_files=grant_budget[rel_path][0], - max_lines=grant_budget[rel_path][1], + max_files=grant_max_files[rel_path], + max_lines=grant_max_lines[rel_path], files=tuple(files), ) for rel_path, files in sorted(grant_files.items()) diff --git a/products/stamphog/packages/pr-approval-agent/test_policy.py b/products/stamphog/packages/pr-approval-agent/test_policy.py index 9a3ec45a034b..c37fb1985e81 100644 --- a/products/stamphog/packages/pr-approval-agent/test_policy.py +++ b/products/stamphog/packages/pr-approval-agent/test_policy.py @@ -503,8 +503,6 @@ def test_resolve_nearest_grant_wins_across_siblings(fake_repo: Path) -> None: def test_resolve_child_inherits_ungranted_key_from_parent(fake_repo: Path) -> None: - # A child granting only max_lines refines its parent's max_files grant: its - # files ride the nearest grant of each key on the chain. _write_agent_policy(fake_repo, "products", _grant(max_files=50), "parent guidance") _write_agent_policy(fake_repo, "products/visual_review", _grant(max_lines=1000), "child guidance") eff = resolve(gates.POLICY, ["products/visual_review/a.py", "products/foo.py"]) From f24f5593257ada15df66bfd92b3cb495759be5cf Mon Sep 17 00:00:00 2001 From: Charles Vien Date: Sat, 29 Aug 2026 23:41:10 -0700 Subject: [PATCH 3/5] Raise desktop line ceiling to 1000 via folder grant --- products/desktop/AGENT_APPROVALS.md | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 products/desktop/AGENT_APPROVALS.md diff --git a/products/desktop/AGENT_APPROVALS.md b/products/desktop/AGENT_APPROVALS.md new file mode 100644 index 000000000000..39b8516fa5eb --- /dev/null +++ b/products/desktop/AGENT_APPROVALS.md @@ -0,0 +1,11 @@ +--- +stamphog: + size_gate: + max_lines: 1000 +--- + +Desktop PRs are often tall: one feature usually lands across packages/core, packages/ui and the app in a single PR, so substantive line counts run past the global ceiling without adding review risk. +A larger line count alone is not a red flag here, so it may be reviewed more leniently than the global default. + +Correctness concerns get the usual full scrutiny: authentication, data handling, and CI or workflow changes are judged exactly as they are anywhere else. +This guidance only relaxes the line ceiling; it never lowers the bar for the deny rules or the refusal criteria. From 14d92f3e4946a44f41102070510466dd157f644c Mon Sep 17 00:00:00 2001 From: Charles Vien Date: Sat, 29 Aug 2026 23:58:50 -0700 Subject: [PATCH 4/5] Budget size-gate lines and files independently A folder that granted one size_gate key was given a fresh copy of the other key's global ceiling, which created a second budget nobody delegated: a lines-only grant let a PR touch 30 folder files plus 30 global files, and a files-only grant let it add 800 folder lines plus 800 global lines. Each ceiling now resolves on its own chain walk. A file counts against the nearest folder granting that ceiling and against the global pool when the chain grants it nowhere, so a one-key grant leaves its files in the pool they were already in for the other key. Generated-By: PostHog Desktop Task-Id: dd27894e-233a-469b-ada8-c80d13d67405 --- .stamphog/README.md | 6 +- .../packages/pr-approval-agent/policy.py | 121 +++++++------- .../packages/pr-approval-agent/review_pr.py | 56 ++++--- .../packages/pr-approval-agent/test_policy.py | 149 +++++++++++------- 4 files changed, 190 insertions(+), 142 deletions(-) diff --git a/.stamphog/README.md b/.stamphog/README.md index f3c109afb28e..93418e5eea98 100644 --- a/.stamphog/README.md +++ b/.stamphog/README.md @@ -21,17 +21,19 @@ A folder may carry an `AGENT_APPROVALS.md` with a `stamphog:` frontmatter block Resolution: - Every `AGENT_APPROVALS.md` at or above a changed file governs it: guidance accumulates outermost first, and a child file adds to its ancestors rather than replacing them. -- For the delegated `size_gate.max_files` and `size_gate.max_lines`, the nearest file on the chain with a valid grant wins for its files (within the contract ceilings). A file that grants only one key inherits the other from the nearest ancestor grant, or from the global value when no ancestor grants it. Files whose chain grants nothing belong to the global pool. +- For the delegated `size_gate.max_files` and `size_gate.max_lines`, the nearest file on the chain with a valid grant wins for its files (within the contract ceilings). Each key resolves on its own: a folder that grants only one key leaves its files to the nearest ancestor grant of the other key, or to the global pool when no ancestor grants it. Files whose chain grants nothing belong to the global pool. - The frontmatter is a positive allow-list: only keys named in the `overrides` contract in `policy.yml` are read, within their ceilings. Anything else (unknown key, out-of-bounds value, unparseable frontmatter) invalidates the whole file - frontmatter and prose. An invalid file contributes nothing itself, but it does not cancel its ancestors: files under it still ride an ancestor's grant, or fall to the global pool if the chain grants nothing. Rationale: an author who can write an invalid file could equally delete it, so treating invalid as absent grants no extra power, and every `AGENT_APPROVALS.md` edit is human-reviewed via the `stamphog_policy` deny anyway. - The prose is untrusted advisory guidance. It is sanitized, length-capped, and injected inside the reviewer prompt's untrusted region; it can never override the deny rules or the refusal criteria. ### Mixed PRs get mixed leniency -Each scope's files are counted against that scope's own line and file ceilings, so a grant covers exactly the files that resolve to it (the nearest valid grant on their chain) and nothing else. +Each scope's files are counted against that scope's own ceiling, so a grant covers exactly the files that resolve to it (the nearest valid grant of that key on their chain) and nothing else. Example: a PR changing 30 files under `products/visual_review/` (ceiling 50) plus 19 files elsewhere (global ceiling 20) passes, because each budget fits. Add a 21st global file and the PR is denied for the global budget, no matter how much headroom the folder still has. Files whose chain grants nothing (no folder file, prose-only, or only invalid grants) count against the global budget, so splitting files across pseudo-scopes can never inflate the allowance. Lines follow the same rule: a scope's substantive lines are counted against that scope's own line ceiling, and the global pool's lines against the global line ceiling. +The two ceilings are budgeted separately, so a folder that raises only the line ceiling still counts its files against the one global file budget. +That keeps a one-key grant from opening a second budget for the key it never asked for. ## Delegation contract diff --git a/products/stamphog/packages/pr-approval-agent/policy.py b/products/stamphog/packages/pr-approval-agent/policy.py index fcfd0fa6bdb5..dbec3e230580 100644 --- a/products/stamphog/packages/pr-approval-agent/policy.py +++ b/products/stamphog/packages/pr-approval-agent/policy.py @@ -182,16 +182,16 @@ def deny_pattern_defs(self) -> dict[str, dict[str, list[str]]]: @dataclass(frozen=True) class ScopeBudget: - """One size-gate budget: a folder override's files, or the global pool. + """One size-gate ceiling and the files it governs. - `path` is the nearest granting AGENT_APPROVALS.md (repo-relative); None is - the global pool, which absorbs every file whose chain grants nothing so - splitting files across pseudo-scopes can never inflate the allowance. + `path` is the nearest AGENT_APPROVALS.md granting that ceiling (repo-relative); + None is the global pool, which absorbs every file whose chain grants that + ceiling nowhere, so splitting files across pseudo-scopes can never inflate + the allowance. """ path: str | None - max_files: int - max_lines: int + ceiling: int files: tuple[str, ...] @@ -200,17 +200,31 @@ class EffectivePolicy: """Per-PR resolved policy: per-scope size budgets plus advisory prose. Mixed PRs get mixed leniency: every AGENT_APPROVALS.md at or above a changed - file governs it, and the file is budgeted by the nearest folder on that - chain with a valid size_gate grant. Each scope's files must fit that scope's - own line and file ceilings; files with no valid grant on their chain keep - the global ceilings. No file ever gets more leniency than its own chain + file governs it, and each ceiling resolves on its own - a file is budgeted by + the nearest folder on its chain granting that ceiling, and by the global pool + when no folder on the chain grants it. Lines and files therefore partition + independently, so a folder that raises one ceiling never opens a second + budget for the other. No file ever gets more leniency than its own chain grants. """ - scopes: tuple[ScopeBudget, ...] + file_scopes: tuple[ScopeBudget, ...] + line_scopes: tuple[ScopeBudget, ...] folder_prose: str | None = None invalid_folder_files: tuple[str, ...] = () + def governed_file_counts(self) -> tuple[tuple[str, int], ...]: + """Changed files each granting AGENT_APPROVALS.md governs, path-sorted. + + A folder can govern a file's line ceiling, its file ceiling, or both, so + each file counts once per folder. + """ + governed: dict[str, set[str]] = {} + for scope in (*self.file_scopes, *self.line_scopes): + if scope.path is not None: + governed.setdefault(scope.path, set()).update(scope.files) + return tuple((path, len(files)) for path, files in sorted(governed.items())) + class PolicyError(ValueError): """Raised when the global policy is malformed - fail closed at load time.""" @@ -510,10 +524,6 @@ class _FolderOverride: prose: str | None = None invalid: bool = False - @property - def grants(self) -> bool: - return self.max_files is not None or self.max_lines is not None - def _scope_chain_for( file_path: str, root: Path, cache: dict[PurePosixPath, tuple[PurePosixPath, ...]] @@ -618,11 +628,12 @@ def _read_delegated_size_gate(stamphog: dict[str, Any], contract: dict[str, Over def resolve(policy: Policy, changed_files: list[str]) -> EffectivePolicy: """Resolve the per-scope size budgets for a PR's changed files. - Every AGENT_APPROVALS.md at or above a changed file governs it. A file's size - budget comes from the nearest folder on its chain with a valid size_gate - grant, and each ceiling in that budget is the nearest grant of that key on - the chain; files whose chain grants nothing (no folder file, prose-only, or - only invalid grants) pool into the global budget. Advisory prose accumulates from + Every AGENT_APPROVALS.md at or above a changed file governs it. Each ceiling + resolves on its own: a file counts against the nearest folder on its chain + granting that ceiling, and against the global pool when the chain grants it + nowhere (no folder file, prose-only, or only invalid grants). Lines and files + therefore partition independently, so a folder raising one ceiling leaves its + files in the global pool for the other. Advisory prose accumulates from every valid folder file on the chain of at least one changed file, outermost first so general guidance precedes specific. An invalid folder file is treated as absent - it grants nothing and adds no prose, but its ancestors @@ -638,44 +649,30 @@ def parsed_for(scope_dir: PurePosixPath) -> tuple[str, _FolderOverride]: parse_cache[scope_dir] = (rel_path, _parse_folder_policy(root / rel_path, policy.overrides)) return parse_cache[scope_dir] - # Files sharing a granting AGENT_APPROVALS.md pool into one budget; the folder - # files touched by any chain feed the prose and invalid-file reporting. - grant_files: dict[str, list[str]] = {} - grant_max_files: dict[str, int] = {} - grant_max_lines: dict[str, int] = {} - global_files: list[str] = [] + # Files sharing a granting AGENT_APPROVALS.md pool into one budget, per + # ceiling; a None bucket key is the global pool. The folder files touched by + # any chain feed the prose and invalid-file reporting. + file_buckets: dict[str | None, list[str]] = {} + line_buckets: dict[str | None, list[str]] = {} + file_ceilings: dict[str, int] = {} + line_ceilings: dict[str, int] = {} on_chain: dict[str, _FolderOverride] = {} # rel path -> parse, each file once for file_path in changed_files: - scope_path: str | None = None - max_files: int | None = None - max_lines: int | None = None + file_scope: str | None = None + line_scope: str | None = None for scope_dir in _scope_chain_for(file_path, root, dir_cache): rel_path, parsed = parsed_for(scope_dir) on_chain[rel_path] = parsed - if parsed.invalid or not parsed.grants: + if parsed.invalid: continue - if scope_path is None: - scope_path = rel_path - if max_files is None: - max_files = parsed.max_files - if max_lines is None: - max_lines = parsed.max_lines - if scope_path is None: - global_files.append(file_path) - continue - grant_files.setdefault(scope_path, []).append(file_path) - grant_max_files[scope_path] = policy.size_gate.max_files if max_files is None else max_files - grant_max_lines[scope_path] = policy.size_gate.max_lines if max_lines is None else max_lines - - override_scopes = [ - ScopeBudget( - path=rel_path, - max_files=grant_max_files[rel_path], - max_lines=grant_max_lines[rel_path], - files=tuple(files), - ) - for rel_path, files in sorted(grant_files.items()) - ] + if file_scope is None and parsed.max_files is not None: + file_scope = rel_path + file_ceilings[rel_path] = parsed.max_files + if line_scope is None and parsed.max_lines is not None: + line_scope = rel_path + line_ceilings[rel_path] = parsed.max_lines + file_buckets.setdefault(file_scope, []).append(file_path) + line_buckets.setdefault(line_scope, []).append(file_path) prose_parts: list[tuple[str, str]] = [] invalid_files: list[str] = [] @@ -695,14 +692,20 @@ def parsed_for(scope_dir: PurePosixPath) -> tuple[str, _FolderOverride]: else: folder_prose = None - global_scope = ScopeBudget( - path=None, - max_files=policy.size_gate.max_files, - max_lines=policy.size_gate.max_lines, - files=tuple(global_files), - ) return EffectivePolicy( - scopes=(*override_scopes, global_scope), + file_scopes=_scope_budgets(file_buckets, file_ceilings, policy.size_gate.max_files), + line_scopes=_scope_budgets(line_buckets, line_ceilings, policy.size_gate.max_lines), folder_prose=folder_prose, invalid_folder_files=tuple(invalid_files), ) + + +def _scope_budgets( + buckets: dict[str | None, list[str]], ceilings: dict[str, int], global_ceiling: int +) -> tuple[ScopeBudget, ...]: + """Folder budgets sorted by path, then the global pool - present even when empty.""" + overrides = [ + ScopeBudget(path=path, ceiling=ceilings[path], files=tuple(buckets[path])) + for path in sorted(path for path in buckets if path is not None) + ] + return (*overrides, ScopeBudget(path=None, ceiling=global_ceiling, files=tuple(buckets.get(None, ())))) diff --git a/products/stamphog/packages/pr-approval-agent/review_pr.py b/products/stamphog/packages/pr-approval-agent/review_pr.py index eda6f3f8f22b..7a6c5e01d35a 100644 --- a/products/stamphog/packages/pr-approval-agent/review_pr.py +++ b/products/stamphog/packages/pr-approval-agent/review_pr.py @@ -681,31 +681,42 @@ def _check_size(self) -> tuple[bool, str]: suffix_parts.append(f"{self.pr.lines_total}L/{len(self.pr.files)}F incl. docs/generated/snapshots") suffix = (", " + "; ".join(suffix_parts)) if suffix_parts else "" # Mixed PRs get mixed leniency: each file counts against the budget of - # the scope governing it (a folder override or the global pool), so a - # folder's higher ceilings cover its own files and nothing else. - for scope in self._size_scopes(): - in_scope = set(scope.files) - scope_lines, scope_files = substantive_size([f for f in self.pr.files if f["filename"] in in_scope]) - where = scope.path or "global" - if scope_lines > scope.max_lines: + # the scope governing it for a given ceiling (a folder override or the + # global pool), so a folder's higher ceiling covers its own files and + # nothing else. Lines and files partition independently, so a folder + # that raises one ceiling keeps the global one for the other. + line_scopes, file_scopes = self._size_scopes() + for scope in line_scopes: + scope_lines, _ = self._scope_size(scope) + if scope_lines > scope.ceiling: return ( False, - f"too large for auto-review ({scope_lines}L substantive in {where} — " - f"ceiling is {scope.max_lines}L; {lines}L, {files}F total{suffix})", + f"too large for auto-review ({scope_lines}L substantive in {scope.path or 'global'} — " + f"ceiling is {scope.ceiling}L; {lines}L, {files}F total{suffix})", ) - if scope_files > scope.max_files: + for scope in file_scopes: + _, scope_files = self._scope_size(scope) + if scope_files > scope.ceiling: return ( False, - f"too large for auto-review ({scope_files}F substantive in {where} — " - f"ceiling is {scope.max_files}F; {lines}L, {files}F total{suffix})", + f"too large for auto-review ({scope_files}F substantive in {scope.path or 'global'} — " + f"ceiling is {scope.ceiling}F; {lines}L, {files}F total{suffix})", ) return True, f"{lines}L, {files}F substantive{suffix} — within ceiling" - def _size_scopes(self) -> tuple[ScopeBudget, ...]: + def _scope_size(self, scope: ScopeBudget) -> tuple[int, int]: + in_scope = set(scope.files) + return substantive_size([f for f in self.pr.files if f["filename"] in in_scope]) + + def _size_scopes(self) -> tuple[tuple[ScopeBudget, ...], tuple[ScopeBudget, ...]]: + """The PR's line budgets and file budgets, in that order.""" if self.effective_policy is not None: - return self.effective_policy.scopes + return self.effective_policy.line_scopes, self.effective_policy.file_scopes all_files = tuple(f["filename"] for f in self.pr.files) - return (ScopeBudget(path=None, max_files=MAX_FILES, max_lines=MAX_LINES, files=all_files),) + return ( + (ScopeBudget(path=None, ceiling=MAX_LINES, files=all_files),), + (ScopeBudget(path=None, ceiling=MAX_FILES, files=all_files),), + ) def _check_tier(self) -> tuple[bool, str]: cl = self.classification @@ -1022,11 +1033,8 @@ def _render_review_body(self) -> str | None: elif thumbs: bullets.append(f"👍 on the PR from {', '.join(thumbs)}.") if self.effective_policy is not None: - for scope in self.effective_policy.scopes: - if scope.path and scope.files: - bullets.append( - f"{len(scope.files)} of the {len(self.pr.files)} changed files are governed by `{scope.path}`." - ) + for path, governed in self.effective_policy.governed_file_counts(): + bullets.append(f"{governed} of the {len(self.pr.files)} changed files are governed by `{path}`.") bullets.extend(str(issue) for issue in (self.reviewer_output.get("issues") or [])[:3]) rows = [f"| {g.gate} | {'✓' if g.passed else '✗'} | {g.message} |" for g in self.gate_results if g] @@ -1081,8 +1089,12 @@ def to_dict(self) -> dict: "policy_file": ".stamphog/policy.yml", "scopes": ( [ - {"path": s.path, "max_files": s.max_files, "max_lines": s.max_lines, "files": len(s.files)} - for s in self.effective_policy.scopes + {"path": s.path, "key": key, "ceiling": s.ceiling, "files": len(s.files)} + for key, scopes in ( + ("max_lines", self.effective_policy.line_scopes), + ("max_files", self.effective_policy.file_scopes), + ) + for s in scopes ] if self.effective_policy else [] diff --git a/products/stamphog/packages/pr-approval-agent/test_policy.py b/products/stamphog/packages/pr-approval-agent/test_policy.py index c37fb1985e81..6b1d2695c723 100644 --- a/products/stamphog/packages/pr-approval-agent/test_policy.py +++ b/products/stamphog/packages/pr-approval-agent/test_policy.py @@ -313,18 +313,27 @@ def fake_repo(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: return tmp_path -def _scope(eff, path): - return next(s for s in eff.scopes if s.path == path) +def _file_scope(eff, path): + return next(s for s in eff.file_scopes if s.path == path) + + +def _line_scope(eff, path): + return next(s for s in eff.line_scopes if s.path == path) def test_resolve_folder_override_budgets_its_own_files(fake_repo: Path) -> None: _write_folder_policy(fake_repo, "stamphog:\n size_gate:\n max_files: 50") - eff = resolve(gates.POLICY, ["products/visual_review/a.py", "products/visual_review/sub/b.py"]) - vr = _scope(eff, _VISUAL_REVIEW_FILE) - assert vr.max_files == 50 - assert set(vr.files) == {"products/visual_review/a.py", "products/visual_review/sub/b.py"} - assert _scope(eff, None).files == () - assert vr.max_lines == gates.MAX_LINES + changed = ["products/visual_review/a.py", "products/visual_review/sub/b.py"] + eff = resolve(gates.POLICY, changed) + vr = _file_scope(eff, _VISUAL_REVIEW_FILE) + assert vr.ceiling == 50 + assert set(vr.files) == set(changed) + assert _file_scope(eff, None).files == () + # Granting max_files alone leaves the lines in the global pool, at the global ceiling. + global_lines = _line_scope(eff, None) + assert global_lines.ceiling == gates.MAX_LINES + assert set(global_lines.files) == set(changed) + assert [s.path for s in eff.line_scopes] == [None] assert eff.invalid_folder_files == () assert eff.folder_prose == "advisory prose" @@ -335,30 +344,41 @@ def test_resolve_mixed_pr_budgets_each_scope_separately(fake_repo: Path) -> None # override, it just has to fit the global budget itself. _write_folder_policy(fake_repo, "stamphog:\n size_gate:\n max_files: 50") eff = resolve(gates.POLICY, ["products/visual_review/a.py", "README.md"]) - assert _scope(eff, _VISUAL_REVIEW_FILE).max_files == 50 - assert _scope(eff, _VISUAL_REVIEW_FILE).files == ("products/visual_review/a.py",) - assert _scope(eff, None).max_files == gates.MAX_FILES - assert _scope(eff, None).files == ("README.md",) + assert _file_scope(eff, _VISUAL_REVIEW_FILE).ceiling == 50 + assert _file_scope(eff, _VISUAL_REVIEW_FILE).files == ("products/visual_review/a.py",) + assert _file_scope(eff, None).ceiling == gates.MAX_FILES + assert _file_scope(eff, None).files == ("README.md",) @pytest.mark.parametrize( - "frontmatter, max_files, max_lines", + "frontmatter, expected_file_budget, expected_line_budget", [ - pytest.param(_grant(max_lines=1000), gates.MAX_FILES, 1000, id="lines-only"), - pytest.param(_grant(max_files=50, max_lines=1000), 50, 1000, id="both"), + pytest.param(_grant(max_files=50), (_VISUAL_REVIEW_FILE, 50), (None, gates.MAX_LINES), id="files-only"), + pytest.param(_grant(max_lines=1000), (None, gates.MAX_FILES), (_VISUAL_REVIEW_FILE, 1000), id="lines-only"), + pytest.param( + _grant(max_files=50, max_lines=1000), + (_VISUAL_REVIEW_FILE, 50), + (_VISUAL_REVIEW_FILE, 1000), + id="both-keys", + ), ], ) -def test_resolve_folder_grant_keeps_global_ceiling_for_ungranted_key( - fake_repo: Path, frontmatter: str, max_files: int, max_lines: int +def test_resolve_budgets_each_ceiling_independently( + fake_repo: Path, + frontmatter: str, + expected_file_budget: tuple[str | None, int], + expected_line_budget: tuple[str | None, int], ) -> None: + # Granting one ceiling must not open a second budget for the other: a + # lines-only folder still counts its files against the one global file + # budget, so it can never double how many files a PR may touch. _write_folder_policy(fake_repo, frontmatter) eff = resolve(gates.POLICY, ["products/visual_review/a.py", "README.md"]) - vr = _scope(eff, _VISUAL_REVIEW_FILE) - assert (vr.max_files, vr.max_lines) == (max_files, max_lines) - assert vr.files == ("products/visual_review/a.py",) - global_scope = _scope(eff, None) - assert (global_scope.max_files, global_scope.max_lines) == (gates.MAX_FILES, gates.MAX_LINES) - assert global_scope.files == ("README.md",) + for (path, ceiling), scopes in ((expected_file_budget, eff.file_scopes), (expected_line_budget, eff.line_scopes)): + governing = next(s for s in scopes if "products/visual_review/a.py" in s.files) + assert (governing.path, governing.ceiling) == (path, ceiling) + # A file outside the folder never rides its grant. + assert next(s for s in scopes if s.path is None).files[-1] == "README.md" @pytest.mark.parametrize( @@ -375,8 +395,9 @@ def test_resolve_folder_grant_keeps_global_ceiling_for_ungranted_key( def test_resolve_invalid_folder_file_pools_files_into_global(fake_repo: Path, frontmatter: str) -> None: _write_folder_policy(fake_repo, frontmatter) eff = resolve(gates.POLICY, ["products/visual_review/a.py"]) - assert [s.path for s in eff.scopes] == [None] - assert _scope(eff, None).files == ("products/visual_review/a.py",) + assert [s.path for s in eff.file_scopes] == [None] + assert [s.path for s in eff.line_scopes] == [None] + assert _file_scope(eff, None).files == ("products/visual_review/a.py",) assert eff.invalid_folder_files == (_VISUAL_REVIEW_FILE,) assert eff.folder_prose is None @@ -384,8 +405,8 @@ def test_resolve_invalid_folder_file_pools_files_into_global(fake_repo: Path, fr @pytest.mark.usefixtures("fake_repo") def test_resolve_no_folder_file_uses_global() -> None: eff = resolve(gates.POLICY, ["posthog/api/insight.py"]) - assert [s.path for s in eff.scopes] == [None] - assert _scope(eff, None).max_files == gates.MAX_FILES + assert [s.path for s in eff.file_scopes] == [None] + assert _file_scope(eff, None).ceiling == gates.MAX_FILES assert eff.invalid_folder_files == () @@ -395,8 +416,8 @@ def test_resolve_prose_only_folder_file_keeps_global_budget(fake_repo: Path) -> (fake_repo / "products" / "visual_review").mkdir(parents=True) (fake_repo / _VISUAL_REVIEW_FILE).write_text("---\n{}\n---\n\nadvice only\n") eff = resolve(gates.POLICY, ["products/visual_review/a.py"]) - assert [s.path for s in eff.scopes] == [None] - assert _scope(eff, None).files == ("products/visual_review/a.py",) + assert [s.path for s in eff.file_scopes] == [None] + assert _file_scope(eff, None).files == ("products/visual_review/a.py",) assert eff.folder_prose == "advice only" @@ -446,15 +467,16 @@ def test_size_gate_applies_mixed_leniency( review_comments=[], check_runs=[], ) + vr_names = tuple(f["filename"] for f in vr_files) + global_names = tuple(f["filename"] for f in global_files) pipeline.effective_policy = EffectivePolicy( - scopes=( - ScopeBudget( - path=_VISUAL_REVIEW_FILE, - max_files=50, - max_lines=1000, - files=tuple(f["filename"] for f in vr_files), - ), - ScopeBudget(path=None, max_files=20, max_lines=500, files=tuple(f["filename"] for f in global_files)), + file_scopes=( + ScopeBudget(path=_VISUAL_REVIEW_FILE, ceiling=50, files=vr_names), + ScopeBudget(path=None, ceiling=20, files=global_names), + ), + line_scopes=( + ScopeBudget(path=_VISUAL_REVIEW_FILE, ceiling=1000, files=vr_names), + ScopeBudget(path=None, ceiling=500, files=global_names), ), ) @@ -480,10 +502,10 @@ def test_resolve_child_rides_nearest_grant_and_accumulates_ancestor_prose( _write_agent_policy(fake_repo, "products", parent_fm, "parent guidance") _write_agent_policy(fake_repo, "products/visual_review", child_fm, "child guidance") eff = resolve(gates.POLICY, ["products/visual_review/a.py"]) - scope = _scope(eff, scope_path) - assert scope.max_files == max_files + scope = _file_scope(eff, scope_path) + assert scope.ceiling == max_files assert scope.files == ("products/visual_review/a.py",) - assert _scope(eff, None).files == () + assert _file_scope(eff, None).files == () assert eff.invalid_folder_files == () assert eff.folder_prose == _multi_prose( (_PRODUCTS_FILE, "parent guidance"), @@ -495,24 +517,29 @@ def test_resolve_nearest_grant_wins_across_siblings(fake_repo: Path) -> None: _write_agent_policy(fake_repo, "products", _grant(30), "parent guidance") _write_agent_policy(fake_repo, "products/visual_review", _grant(50), "child guidance") eff = resolve(gates.POLICY, ["products/visual_review/a.py", "products/foo.py"]) - assert _scope(eff, _VISUAL_REVIEW_FILE).max_files == 50 - assert _scope(eff, _VISUAL_REVIEW_FILE).files == ("products/visual_review/a.py",) - assert _scope(eff, _PRODUCTS_FILE).max_files == 30 - assert _scope(eff, _PRODUCTS_FILE).files == ("products/foo.py",) - assert _scope(eff, None).files == () + assert _file_scope(eff, _VISUAL_REVIEW_FILE).ceiling == 50 + assert _file_scope(eff, _VISUAL_REVIEW_FILE).files == ("products/visual_review/a.py",) + assert _file_scope(eff, _PRODUCTS_FILE).ceiling == 30 + assert _file_scope(eff, _PRODUCTS_FILE).files == ("products/foo.py",) + assert _file_scope(eff, None).files == () -def test_resolve_child_inherits_ungranted_key_from_parent(fake_repo: Path) -> None: +def test_resolve_walks_the_chain_separately_for_each_ceiling(fake_repo: Path) -> None: + # The child grants lines only, so its files still share the parent's one file + # budget rather than getting a second one of their own. _write_agent_policy(fake_repo, "products", _grant(max_files=50), "parent guidance") _write_agent_policy(fake_repo, "products/visual_review", _grant(max_lines=1000), "child guidance") eff = resolve(gates.POLICY, ["products/visual_review/a.py", "products/foo.py"]) - child = _scope(eff, _VISUAL_REVIEW_FILE) - assert (child.max_files, child.max_lines) == (50, 1000) - assert child.files == ("products/visual_review/a.py",) - parent = _scope(eff, _PRODUCTS_FILE) - assert (parent.max_files, parent.max_lines) == (50, gates.MAX_LINES) - assert parent.files == ("products/foo.py",) - assert _scope(eff, None).files == () + parent_files = _file_scope(eff, _PRODUCTS_FILE) + assert parent_files.ceiling == 50 + assert set(parent_files.files) == {"products/visual_review/a.py", "products/foo.py"} + assert [s.path for s in eff.file_scopes] == [_PRODUCTS_FILE, None] + child_lines = _line_scope(eff, _VISUAL_REVIEW_FILE) + assert child_lines.ceiling == 1000 + assert child_lines.files == ("products/visual_review/a.py",) + global_lines = _line_scope(eff, None) + assert global_lines.ceiling == gates.MAX_LINES + assert global_lines.files == ("products/foo.py",) def test_resolve_invalid_child_rides_parent_grant(fake_repo: Path) -> None: @@ -521,10 +548,10 @@ def test_resolve_invalid_child_rides_parent_grant(fake_repo: Path) -> None: _write_agent_policy(fake_repo, "products", _grant(30), "parent guidance") _write_agent_policy(fake_repo, "products/visual_review", _grant(99), "child guidance") eff = resolve(gates.POLICY, ["products/visual_review/a.py"]) - parent_scope = _scope(eff, _PRODUCTS_FILE) - assert parent_scope.max_files == 30 + parent_scope = _file_scope(eff, _PRODUCTS_FILE) + assert parent_scope.ceiling == 30 assert parent_scope.files == ("products/visual_review/a.py",) - assert _scope(eff, None).files == () + assert _file_scope(eff, None).files == () assert eff.invalid_folder_files == (_VISUAL_REVIEW_FILE,) assert eff.folder_prose == "parent guidance" @@ -609,9 +636,13 @@ def _body_pipeline(fam) -> "review_pr.Pipeline": "assurance": {"head_approvals": [], "head_commented_users": ["greptile-apps[bot]"]}, } pipeline.effective_policy = EffectivePolicy( - scopes=( - ScopeBudget(path=_VISUAL_REVIEW_FILE, max_files=50, max_lines=1000, files=("products/visual_review/a.py",)), - ScopeBudget(path=None, max_files=20, max_lines=500, files=()), + file_scopes=( + ScopeBudget(path=_VISUAL_REVIEW_FILE, ceiling=50, files=("products/visual_review/a.py",)), + ScopeBudget(path=None, ceiling=20, files=()), + ), + line_scopes=( + ScopeBudget(path=_VISUAL_REVIEW_FILE, ceiling=1000, files=("products/visual_review/a.py",)), + ScopeBudget(path=None, ceiling=500, files=()), ), ) pipeline.gate_results = [review_pr.GateResult("size", True, "4L, 1F substantive")] From c0e8b7e91721ba5753b9ee0ffd8ba7ecc98e1be2 Mon Sep 17 00:00:00 2001 From: Charles Vien Date: Sun, 30 Aug 2026 00:13:00 -0700 Subject: [PATCH 5/5] Return the size budgets as a policy, not a tuple pair The size-gate helpers returned same-typed pairs, which the tuple-return-prefer-dataclass rule blocks: callers can swap the two scope tuples, or the two counts, without a type error. _size_budgets now returns the EffectivePolicy the scopes already live on, and the per-scope file filter returns the file list so substantive_size stays the one place that unpacks a count pair. Generated-By: PostHog Desktop Task-Id: dd27894e-233a-469b-ada8-c80d13d67405 --- .../packages/pr-approval-agent/review_pr.py | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/products/stamphog/packages/pr-approval-agent/review_pr.py b/products/stamphog/packages/pr-approval-agent/review_pr.py index 7a6c5e01d35a..54ada8f12a6b 100644 --- a/products/stamphog/packages/pr-approval-agent/review_pr.py +++ b/products/stamphog/packages/pr-approval-agent/review_pr.py @@ -685,17 +685,17 @@ def _check_size(self) -> tuple[bool, str]: # global pool), so a folder's higher ceiling covers its own files and # nothing else. Lines and files partition independently, so a folder # that raises one ceiling keeps the global one for the other. - line_scopes, file_scopes = self._size_scopes() - for scope in line_scopes: - scope_lines, _ = self._scope_size(scope) + budgets = self._size_budgets() + for scope in budgets.line_scopes: + scope_lines, _ = substantive_size(self._files_in(scope)) if scope_lines > scope.ceiling: return ( False, f"too large for auto-review ({scope_lines}L substantive in {scope.path or 'global'} — " f"ceiling is {scope.ceiling}L; {lines}L, {files}F total{suffix})", ) - for scope in file_scopes: - _, scope_files = self._scope_size(scope) + for scope in budgets.file_scopes: + _, scope_files = substantive_size(self._files_in(scope)) if scope_files > scope.ceiling: return ( False, @@ -704,18 +704,18 @@ def _check_size(self) -> tuple[bool, str]: ) return True, f"{lines}L, {files}F substantive{suffix} — within ceiling" - def _scope_size(self, scope: ScopeBudget) -> tuple[int, int]: + def _files_in(self, scope: ScopeBudget) -> list[dict]: in_scope = set(scope.files) - return substantive_size([f for f in self.pr.files if f["filename"] in in_scope]) + return [f for f in self.pr.files if f["filename"] in in_scope] - def _size_scopes(self) -> tuple[tuple[ScopeBudget, ...], tuple[ScopeBudget, ...]]: - """The PR's line budgets and file budgets, in that order.""" + def _size_budgets(self) -> EffectivePolicy: + """The PR's resolved size budgets, or global-only ones when resolution did not run.""" if self.effective_policy is not None: - return self.effective_policy.line_scopes, self.effective_policy.file_scopes + return self.effective_policy all_files = tuple(f["filename"] for f in self.pr.files) - return ( - (ScopeBudget(path=None, ceiling=MAX_LINES, files=all_files),), - (ScopeBudget(path=None, ceiling=MAX_FILES, files=all_files),), + return EffectivePolicy( + file_scopes=(ScopeBudget(path=None, ceiling=MAX_FILES, files=all_files),), + line_scopes=(ScopeBudget(path=None, ceiling=MAX_LINES, files=all_files),), ) def _check_tier(self) -> tuple[bool, str]: