Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions .stamphog/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,24 @@ Lines follow the same rule: a scope's substantive lines are counted against that
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.

### The roof bounds the whole PR

Per-scope budgets alone would let a PR's total grow with the number of scopes it touches.
A folder granting 1000 lines next to the 800-line global pool would allow 1800, and every further granting folder would add its own budget on top.
So each ceiling also carries a roof over the whole PR: the most generous ceiling in play for that key.
A PR touching `products/desktop/` gets a 1000-line roof, whatever else it touches.

The roof needs no separate number in `policy.yml`.
Every grant is validated at or under the contract ceiling, and the global pool is always a scope, so the roof stays between the global default and the contract ceiling.
With no grant in play it equals the global default, which is the single global total the gate applied before the ceilings became delegable.

The roof takes no headroom away from a scope.
The per-scope budgets still hold, so the extra lines a folder's grant unlocks are only spendable inside that folder.

## Delegation contract

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).
A ceiling therefore bounds two things: the largest value a folder may grant, and the highest a PR's roof can ever go for that key.
It is not the limit every PR gets. A PR whose files reach no grant keeps the lower global roof.
The loader rejects a ceiling under its own global default, which would otherwise bound nothing.
deny, allow, dismiss, and tiers are non-delegable by construction - they are absent from the contract and cannot be granted from a folder file.
4 changes: 4 additions & 0 deletions products/stamphog/packages/pr-approval-agent/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ Size ceiling (hard gate)
~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)
- The whole PR still has to fit the most generous ceiling in play, so
per-scope budgets never sum. With no folder grant that roof is the global
ceiling above, so the gate keeps measuring the PR size these limits were
derived from
- 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
Expand Down
46 changes: 44 additions & 2 deletions products/stamphog/packages/pr-approval-agent/policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -206,13 +206,33 @@
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.

Per-scope budgets alone would let a PR's total grow with the number of
scopes it touches, so each ceiling also carries a roof over the whole PR.
"""

file_scopes: tuple[ScopeBudget, ...]
line_scopes: tuple[ScopeBudget, ...]
folder_prose: str | None = None
invalid_folder_files: tuple[str, ...] = ()

@property
def line_roof(self) -> int:
"""Whole-PR substantive line ceiling: the most generous one in play.

Every grant is validated at or under the delegation ceiling, the global
pool is always a scope, and the loader rejects a ceiling under its own
global default, so the roof stays between the global default and that
ceiling. With no grant in play it equals the global default, which is
the single global total the gate had before ceilings became delegable.
"""
return max(scope.ceiling for scope in self.line_scopes)
Comment thread
webjunkie marked this conversation as resolved.

@property
def file_roof(self) -> int:
"""Whole-PR substantive file ceiling. See `line_roof`."""
return max(scope.ceiling for scope in self.file_scopes)

def governed_file_counts(self) -> tuple[tuple[str, int], ...]:
"""Changed files each granting AGENT_APPROVALS.md governs, path-sorted.

Expand Down Expand Up @@ -377,6 +397,25 @@
return overrides


def _require_ceilings_cover_globals(size_gate: SizeGate, overrides: dict[str, OverrideContract]) -> None:
"""A delegation ceiling under its own global default is incoherent - fail closed.

The ceiling is what bounds a PR's whole-PR roof. Under the global default it
would bound nothing, because the always-present global pool already carries
the roof past it, and delegation could then only ever lower a folder's
allowance.
"""
globals_by_key = {"size_gate.max_lines": size_gate.max_lines, "size_gate.max_files": size_gate.max_files}
for key, contract in overrides.items():
global_value = globals_by_key.get(key)
if global_value is None:
continue
_require(
contract.ceiling >= global_value,
f"overrides.{key}.ceiling: {contract.ceiling} is under the global {key} ({global_value})",
)


_FAMILIARITY_STRONG_KEYS = {"min_blame_overlap_pct"}
_FAMILIARITY_MODERATE_KEYS = {"min_prior_prs", "max_days_since_touch"}

Expand Down Expand Up @@ -494,14 +533,17 @@
_require(raw["version"] == 1, f"policy version: unsupported version {raw['version']!r}")

path_patterns, extensions = _parse_allow(raw["allow"])
size_gate = _parse_size_gate(raw["size_gate"])
overrides = _parse_overrides(raw["overrides"])
_require_ceilings_cover_globals(size_gate, overrides)
return Policy(
version=1,
deny=_parse_deny(raw["deny"], lockfile_names),
allow_path_patterns=path_patterns,
allow_extensions=extensions,
size_gate=_parse_size_gate(raw["size_gate"]),
size_gate=size_gate,
t1_subclasses=_parse_tiers(raw["tiers"]),
overrides=_parse_overrides(raw["overrides"]),
overrides=overrides,
familiarity=_parse_familiarity(raw["familiarity"]),
ownership=_parse_ownership(raw["ownership"], ownership_formats),
)
Expand Down Expand Up @@ -625,7 +667,7 @@
return grants


def resolve(policy: Policy, changed_files: list[str]) -> EffectivePolicy:

Check warning on line 670 in products/stamphog/packages/pr-approval-agent/policy.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

lint:complexity

`resolve` has cyclomatic complexity 13 (warn >10)

Check warning on line 670 in products/stamphog/packages/pr-approval-agent/policy.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

`resolve` has cyclomatic complexity 13 (warn >10)
"""Resolve the per-scope size budgets for a PR's changed files.

Every AGENT_APPROVALS.md at or above a changed file governs it. Each ceiling
Expand Down
16 changes: 15 additions & 1 deletion products/stamphog/packages/pr-approval-agent/review_pr.py
Original file line number Diff line number Diff line change
Expand Up @@ -684,7 +684,9 @@ def _check_size(self) -> tuple[bool, str]:
# 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.
# that raises one ceiling keeps the global one for the other. Each
# ceiling then has a roof over the PR total, so scope budgets cannot sum
# without bound as more folders grant.
budgets = self._size_budgets()
for scope in budgets.line_scopes:
scope_lines, _ = substantive_size(self._files_in(scope))
Expand All @@ -702,6 +704,18 @@ def _check_size(self) -> tuple[bool, str]:
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})",
)
if lines > budgets.line_roof:
return (
False,
f"too large for auto-review ({lines}L, {files}F substantive across the whole PR — "
f"roof is {budgets.line_roof}L{suffix})",
)
if files > budgets.file_roof:
return (
Comment thread
webjunkie marked this conversation as resolved.
False,
f"too large for auto-review ({lines}L, {files}F substantive across the whole PR — "
f"roof is {budgets.file_roof}F{suffix})",
)
return True, f"{lines}L, {files}F substantive{suffix} — within ceiling"

def _files_in(self, scope: ScopeBudget) -> list[dict]:
Expand Down
101 changes: 78 additions & 23 deletions products/stamphog/packages/pr-approval-agent/test_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,10 @@ def _out_of_contract_delegation(d: dict) -> None:
d["overrides"]["deny"] = {"ceiling": 1}


def _ceiling_under_global_default(d: dict) -> None:
d["size_gate"]["max_lines"] = d["overrides"]["size_gate.max_lines"]["ceiling"] + 1


def _rename_deps_toolchain(d: dict) -> None:
d["deny"]["dependencies_toolchain"] = d["deny"].pop("deps_toolchain")

Expand Down Expand Up @@ -246,6 +250,7 @@ def _ownership_wrong_locator_for_format(d: dict) -> None:
_invalid_regex,
_drop_self_governance,
_out_of_contract_delegation,
_ceiling_under_global_default,
_rename_deps_toolchain,
_ownership_unknown_format,
_ownership_both_locators,
Expand Down Expand Up @@ -410,6 +415,27 @@ def test_resolve_no_folder_file_uses_global() -> None:
assert eff.invalid_folder_files == ()


@pytest.mark.parametrize(
"frontmatter, expected_line_roof",
[
pytest.param(None, gates.MAX_LINES, id="no-grant-keeps-the-global-total"),
pytest.param(_grant(max_lines=200), gates.MAX_LINES, id="grant-under-the-global-default"),
pytest.param(_grant(max_lines=1000), 1000, id="grant-over-the-global-default"),
],
)
def test_roof_is_the_most_generous_ceiling_in_play(
fake_repo: Path, frontmatter: str | None, expected_line_roof: int
) -> None:
# A folder may grant under the global default, so the roof reads the global
# pool too. It is always a scope, which keeps the roof from dropping below
# the global ceiling.
if frontmatter is not None:
_write_folder_policy(fake_repo, frontmatter)
eff = resolve(gates.POLICY, ["products/visual_review/a.py"])
assert eff.line_roof == expected_line_roof
assert eff.file_roof == gates.MAX_FILES


def test_resolve_prose_only_folder_file_keeps_global_budget(fake_repo: Path) -> None:
# No pseudo-scope budget: without a max_files grant the files pool into
# the global budget, but the advisory prose still reaches the reviewer.
Expand All @@ -427,28 +453,8 @@ def test_resolve_carries_sanitized_prose(fake_repo: Path) -> None:
assert eff.folder_prose == "keepthis"


@pytest.mark.parametrize(
"vr_additions, global_additions, n_global, expected_ok, expected_where",
[
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(
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)
]

def _size_pipeline(vr_files: list[dict], global_files: list[dict]) -> "review_pr.Pipeline":
# The folder scope carries the higher ceiling on both keys.
pipeline = review_pr.Pipeline(pr_number=1, repo="PostHog/posthog")
pipeline.pr = PRData(
number=1,
Expand Down Expand Up @@ -479,13 +485,62 @@ def test_size_gate_applies_mixed_leniency(
ScopeBudget(path=None, ceiling=500, files=global_names),
),
)
return pipeline

ok, message = pipeline._check_size()

@pytest.mark.parametrize(
"vr_additions, global_additions, n_global, expected_ok, expected_where",
[
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(
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)
]

ok, message = _size_pipeline(vr_files, global_files)._check_size()
assert ok is expected_ok
if expected_where is not None:
assert f"in {expected_where}" in message


@pytest.mark.parametrize(
"n_vr, vr_additions, n_global, global_additions, expected_roof",
[
pytest.param(30, 30, 19, 26, "1000L", id="line-roof"),
pytest.param(45, 1, 19, 1, "50F", id="file-roof"),
],
)
def test_size_gate_roof_bounds_the_whole_pr(
n_vr: int, vr_additions: int, n_global: int, global_additions: int, expected_roof: str
) -> None:
# Every scope fits its own budget here. Without a roof the PR total would
# grow with the number of granting folders it touches.
vr_files = [
{"filename": f"products/visual_review/f{i}.py", "additions": vr_additions, "deletions": 0} for i in range(n_vr)
]
global_files = [
{"filename": f"posthog/api/m{i}.py", "additions": global_additions, "deletions": 0} for i in range(n_global)
]

ok, message = _size_pipeline(vr_files, global_files)._check_size()
assert ok is False
assert "across the whole PR" in message
assert f"roof is {expected_roof}" in message


@pytest.mark.parametrize(
"parent_fm, child_fm, scope_path, max_files",
[
Expand Down
Loading