From c6a92953b05f57bb88e220f7f12db4a9fa9c5d8a Mon Sep 17 00:00:00 2001 From: Sudhanshu Sali Date: Tue, 7 Jul 2026 10:33:26 -0400 Subject: [PATCH] feat: configurable allowed_labels + brand rate-limit label with bot_name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two engine changes surfaced by the deequ/python-deequ/dqdl migrations: 1. allowed_labels was a hardcoded set, so issue-triage dropped repo-specific labels (python-deequ's 'python', dqdl's 'java'). Now reads from .shadow.yml bot.allowed_labels (list) or BOT_ALLOWED_LABELS env (comma-separated), each entry scrubbed like any posted label. Empty/garbage falls back to the original default set, so existing deployments are unchanged. 2. The rate-limit escalation label was a hardcoded 'shadow:rate-limited' literal that ignored bot.name. Now branded f'{bot_name}:rate-limited', and appended in act() AFTER the allowed_labels filter (like escalate_label) so it actually reaches GitHub — an engine-generated label must not be dropped by the model-proposed-label allowlist. Adds tests/unit/test_allowed_labels.py (11 cases incl. an end-to-end act() test asserting the branded label reaches add_labels) + README updates. Full suite 538 pass. --- README.md | 10 +- src/scripts/shadow/config.py | 36 +++++- src/scripts/shadow/main.py | 9 +- tests/unit/test_allowed_labels.py | 179 ++++++++++++++++++++++++++++++ 4 files changed, 228 insertions(+), 6 deletions(-) create mode 100644 tests/unit/test_allowed_labels.py diff --git a/README.md b/README.md index e857f85..b890d74 100644 --- a/README.md +++ b/README.md @@ -187,6 +187,11 @@ bot: escalate_label: needs-human # must already exist in your repo's Labels max_replies: 2 # 0..100; cap on followup replies before next user comment ESCALATEs max_runs_per_hour: 20 # 0..100; per-(repo, item) rate limit; 0 disables + allowed_labels: # labels issue-triage may apply; defaults to bug/enhancement/question/documentation/help-wanted. Add repo-specific labels here (they must exist in your repo's Labels). + - bug + - enhancement + - question + - documentation # Override per-stage models if you want a different cost/quality trade-off. # Env vars BEDROCK_MODEL_ID / BEDROCK_REPORTER_MODEL_ID / BEDROCK_CRITIC_MODEL_ID / @@ -213,7 +218,8 @@ models: | `BOT_REPORTER_MIN_REMAINING_S` | `60` | Reporter is pre-empted if less than this remains. | | `BOT_MAX_DIFF_FOR_REVIEW_CHARS` | `100000` | Pre-flight diff cap. Diff above this → ESCALATE before any Bedrock call. | | `BOT_MAX_FILES_FOR_REVIEW` | `50` | Pre-flight file-count cap. Same shape as above. | -| `BOT_MAX_RUNS_PER_HOUR` | `20` | Per-(repo, item) hourly run cap. `0` disables. Capped at `100`; values above clamp with a warning. Negative values fall back to default. Hit → ESCALATE with `shadow:rate-limited` label. | +| `BOT_MAX_RUNS_PER_HOUR` | `20` | Per-(repo, item) hourly run cap. `0` disables. Capped at `100`; values above clamp with a warning. Negative values fall back to default. Hit → ESCALATE with a `:rate-limited` label. | +| `BOT_ALLOWED_LABELS` | `bug,enhancement,question,documentation,help-wanted` | Comma-separated allowlist of labels issue-triage may apply (also settable as a list under `.shadow.yml` `bot.allowed_labels`). Model-proposed labels outside the set are dropped. Empty/garbage falls back to the default. | | `BOT_MAX_REPLIES` | `2` | Followup-reply cap per (issue, PR). Capped at `100`; negative falls back to default. | | `BOT_GITHUB_ACTOR` | `github-actions[bot]` | GitHub login Shadow's comments appear under. **Set this to a unique value** if your repo has other workflows that also post as `github-actions[bot]` (e.g., PR-overlap detectors, claim-checkers). Otherwise Shadow's `already_commented` dedup matches their comments and silently SKIPs every PR. | | `BOT_REQUIRE_GUARDRAIL` | `true` | Production runs (`DRY_RUN=false`) refuse to start when `GUARDRAIL_ID` is unset — Shadow won't run without prompt-injection defense. On the reusable workflow, drive this via the `require_guardrail` **input** (`with: require_guardrail: 'false'`) — accepts `0`/`false`/`no`/`off`. `DRY_RUN=true` bypasses the gate regardless. | @@ -411,7 +417,7 @@ Every analyze run writes a `shadow_result.json` artifact, retained 7 days by Git The per-PR levers (under [What it costs](#what-it-costs)) bound a single review. These three guards bound **fleet-wide** spend, defending against PR/issue spam and runaway traffic: -- **Per-(repo, item) hourly rate limit** (`BOT_MAX_RUNS_PER_HOUR`, default `20`). Caps how many times a single PR or issue can trigger Shadow per rolling hour. Beyond the limit, the bot ESCALATES with the `shadow:rate-limited` label instead of running the agent pipeline. Defends against an adversary closing/reopening or editing a PR title in a loop. Set to `0` to disable. **Issue/issue_comment events** require `run-name: "Shadow #${{ github.event.issue.number || ... }}"` in your caller workflow so the rate-limit gate can match prior runs (see [`examples/caller-workflow.yml`](examples/caller-workflow.yml)). +- **Per-(repo, item) hourly rate limit** (`BOT_MAX_RUNS_PER_HOUR`, default `20`). Caps how many times a single PR or issue can trigger Shadow per rolling hour. Beyond the limit, the bot ESCALATES with a `:rate-limited` label instead of running the agent pipeline. Defends against an adversary closing/reopening or editing a PR title in a loop. Set to `0` to disable. **Issue/issue_comment events** require `run-name: "Shadow #${{ github.event.issue.number || ... }}"` in your caller workflow so the rate-limit gate can match prior runs (see [`examples/caller-workflow.yml`](examples/caller-workflow.yml)). - **Pre-flight diff/file caps** (`BOT_MAX_DIFF_FOR_REVIEW_CHARS`, `BOT_MAX_FILES_FOR_REVIEW`, defaults `100000` / `50`). A 50-file PR makes the Investigator read 5+ files, the Critic re-reads, the Reporter formats — costs multiply. Diff or file count above the cap → ESCALATE before any Bedrock call. Pre-flight escalation is ~$0; a runaway pipeline on a giant PR is $5+. - **AWS Budgets opt-in via CFN** (`MonthlyBudgetLimit` parameter on `shadow-iam-stack.yaml`). Set a positive USD amount + a `BudgetEmailAddress` and the stack creates an `AWS::Budgets::Budget` filtered to Amazon Bedrock spend, with email alerts at 80% and 100%. `0` skips Budget creation (default — AWS Budgets bills $0.02/budget/day, so opt-in only). Email-only today; auto-shutdown via `SHADOW_DISABLED` is a planned upgrade. diff --git a/src/scripts/shadow/config.py b/src/scripts/shadow/config.py index 08d46c6..91b58b4 100644 --- a/src/scripts/shadow/config.py +++ b/src/scripts/shadow/config.py @@ -209,9 +209,12 @@ def __init__(self): self.max_context_chars = 800000 self.max_github_search_results = 8 self.github_api_timeout = 10 - self.allowed_labels = { - "bug", "enhancement", "question", "documentation", "help-wanted", - } + # Extensible so adopters can allow repo-specific labels (e.g. python, java). + self.allowed_labels = _parse_allowed_labels( + os.getenv("BOT_ALLOWED_LABELS"), + shadow_config.get(yml, "bot", "allowed_labels"), + _DEFAULT_ALLOWED_LABELS, + ) # Default ON. BOT_AGENT_PIPELINE=0 makes PR events ESCALATE with # reason `pipeline_disabled`; issue/followup paths still run their @@ -326,6 +329,33 @@ def _scrub_label(val, default): return val[:50] +_DEFAULT_ALLOWED_LABELS = frozenset({ + "bug", "enhancement", "question", "documentation", "help-wanted", +}) + + +def _parse_allowed_labels(env_val, yaml_val, default): + """env (comma-separated) > yaml (list) > default. Entries are scrubbed like + posted labels; an empty/all-garbage result falls back to `default` rather + than silently disabling labeling.""" + items = None + if env_val is not None and env_val.strip(): + items = env_val.split(",") + elif isinstance(yaml_val, (list, tuple)): + items = yaml_val + elif isinstance(yaml_val, str) and yaml_val.strip(): + # tolerate a scalar yaml string ("python,java") like the env form + items = yaml_val.split(",") + if items is None: + return set(default) + _sentinel = object() + scrubbed = { + lbl for lbl in (_scrub_label(x, _sentinel) for x in items) + if lbl is not _sentinel + } + return scrubbed or set(default) + + def _int_in_range_or_default(val, default, *, name): """Yaml int, env str, or wrong type → default. Cap range [0, 100] so a yaml typo can't set the bound absurdly high. `0` is valid and means diff --git a/src/scripts/shadow/main.py b/src/scripts/shadow/main.py index c20800e..1e0dd25 100644 --- a/src/scripts/shadow/main.py +++ b/src/scripts/shadow/main.py @@ -221,9 +221,10 @@ def analyze(): if recent is not None and recent >= cfg.max_runs_per_hour: logger.warning("Rate limit hit on #%s: %d runs in last hour (cap %d)", number, recent, cfg.max_runs_per_hour) + # act() adds the branded rate-limit label (keyed on this reason). _write_artifact({ "action": "ESCALATE", - "labels": [cfg.escalate_label, "shadow:rate-limited"], + "labels": [cfg.escalate_label], "response": "", "reason": "rate_limited", "title": title, "html_url": html_url, "number": number, "is_pr": is_pr, @@ -584,6 +585,12 @@ def act(): escalate_labels = list(labels) if cfg.escalate_label and cfg.escalate_label not in escalate_labels: escalate_labels.append(cfg.escalate_label) + # Appended after the filter (like escalate_label): engine-generated, so + # not subject to the model-proposed-label allowlist. Branded with bot_name. + if reason == "rate_limited": + rate_label = f"{cfg.bot_name}:rate-limited" + if rate_label not in escalate_labels: + escalate_labels.append(rate_label) posted_status = { "comment": bool(gh.post_comment(number, ack)), "labels": bool(gh.add_labels(number, escalate_labels)), diff --git a/tests/unit/test_allowed_labels.py b/tests/unit/test_allowed_labels.py new file mode 100644 index 0000000..29190df --- /dev/null +++ b/tests/unit/test_allowed_labels.py @@ -0,0 +1,179 @@ +"""cfg.allowed_labels configurability + rate-limit label branding. + +The issue-triage label allowlist was hardcoded, so adopters could not apply +repo-specific labels (e.g. python-deequ's `python`, dqdl's `java`) — the +engine dropped them at main.py before the Labels API call. Make it read from +.shadow.yml bot.allowed_labels (list) or BOT_ALLOWED_LABELS env (comma-sep), +defaulting to the original set so existing deployments are unchanged. + +Also verifies the rate-limit escalation label tracks bot_name rather than a +hardcoded `shadow:` literal. +""" +import importlib +import tempfile +from pathlib import Path + +import pytest + + +REQUIRED_ENV = { + "GITHUB_TOKEN": "fake-token", + "EVENT_TYPE": "pull_request_target", + "ISSUE_NUMBER": "42", + "GITHUB_REPOSITORY": "owner/repo", + "BOT_REQUIRE_GUARDRAIL": "false", +} + +_DEFAULT = {"bug", "enhancement", "question", "documentation", "help-wanted"} + + +def _set_required_env(monkeypatch): + for k, v in REQUIRED_ENV.items(): + monkeypatch.setenv(k, v) + monkeypatch.delenv("BOT_ALLOWED_LABELS", raising=False) + + +@pytest.fixture(autouse=True) +def _reload_config(): + """Config reads env at import-time; force fresh load per test.""" + import shadow.config # noqa: F401 + importlib.reload(__import__("shadow.config", fromlist=["Config"])) + yield + + +def _cfg(monkeypatch, tmp, yml=None): + if yml is not None: + Path(tmp, ".shadow.yml").write_text(yml) + monkeypatch.setenv("SHADOW_REPO_ROOT", tmp) + from shadow.config import Config + return Config() + + +def test_default_allowed_labels(monkeypatch): + _set_required_env(monkeypatch) + with tempfile.TemporaryDirectory() as tmp: + cfg = _cfg(monkeypatch, tmp) + assert cfg.allowed_labels == _DEFAULT + + +def test_yaml_list_extends_labels(monkeypatch): + _set_required_env(monkeypatch) + with tempfile.TemporaryDirectory() as tmp: + cfg = _cfg(monkeypatch, tmp, + "bot:\n allowed_labels:\n - bug\n - python\n - java\n") + assert cfg.allowed_labels == {"bug", "python", "java"} + + +def test_env_overrides_yaml(monkeypatch): + _set_required_env(monkeypatch) + with tempfile.TemporaryDirectory() as tmp: + Path(tmp, ".shadow.yml").write_text( + "bot:\n allowed_labels:\n - bug\n" + ) + monkeypatch.setenv("SHADOW_REPO_ROOT", tmp) + monkeypatch.setenv("BOT_ALLOWED_LABELS", "enhancement,question") + from shadow.config import Config + cfg = Config() + assert cfg.allowed_labels == {"enhancement", "question"} + + +def test_env_comma_separated(monkeypatch): + _set_required_env(monkeypatch) + with tempfile.TemporaryDirectory() as tmp: + monkeypatch.setenv("BOT_ALLOWED_LABELS", " bug , python ,java") + monkeypatch.setenv("SHADOW_REPO_ROOT", tmp) + from shadow.config import Config + cfg = Config() + # entries are whitespace-stripped by the label scrub + assert cfg.allowed_labels == {"bug", "python", "java"} + + +def test_empty_yaml_list_falls_back_to_default(monkeypatch): + """An empty list must not disable labeling entirely.""" + _set_required_env(monkeypatch) + with tempfile.TemporaryDirectory() as tmp: + cfg = _cfg(monkeypatch, tmp, "bot:\n allowed_labels: []\n") + assert cfg.allowed_labels == _DEFAULT + + +def test_all_garbage_entries_fall_back(monkeypatch): + """A list of only non-string / blank entries scrubs to empty -> default.""" + _set_required_env(monkeypatch) + with tempfile.TemporaryDirectory() as tmp: + cfg = _cfg(monkeypatch, tmp, + "bot:\n allowed_labels:\n - 123\n - \" \"\n - null\n") + assert cfg.allowed_labels == _DEFAULT + + +def test_garbage_scalar_yaml_falls_back(monkeypatch): + """A non-list, non-string scalar (int) drops to default rather than crash.""" + _set_required_env(monkeypatch) + with tempfile.TemporaryDirectory() as tmp: + cfg = _cfg(monkeypatch, tmp, "bot:\n allowed_labels: 5\n") + assert cfg.allowed_labels == _DEFAULT + + +def test_mixed_valid_and_garbage_keeps_valid(monkeypatch): + """Valid entries survive; garbage ones drop (partial list is not all-or-nothing).""" + _set_required_env(monkeypatch) + with tempfile.TemporaryDirectory() as tmp: + cfg = _cfg(monkeypatch, tmp, + "bot:\n allowed_labels:\n - python\n - 123\n - \"\"\n") + assert cfg.allowed_labels == {"python"} + + +def test_scalar_yaml_string_is_split(monkeypatch): + """A scalar yaml string 'a,b' is treated like the comma-sep env form.""" + _set_required_env(monkeypatch) + with tempfile.TemporaryDirectory() as tmp: + cfg = _cfg(monkeypatch, tmp, "bot:\n allowed_labels: \"python,java\"\n") + assert cfg.allowed_labels == {"python", "java"} + + +def test_parse_helper_directly(): + """Unit-level: the pure helper, independent of env loading.""" + from shadow.config import _parse_allowed_labels, _DEFAULT_ALLOWED_LABELS + d = _DEFAULT_ALLOWED_LABELS + assert _parse_allowed_labels(None, None, d) == set(d) + assert _parse_allowed_labels("python,java", None, d) == {"python", "java"} + assert _parse_allowed_labels(None, ["bug"], d) == {"bug"} + assert _parse_allowed_labels(" ", None, d) == set(d) # blank env -> default + assert _parse_allowed_labels(None, [], d) == set(d) # empty list -> default + assert _parse_allowed_labels("python", ["bug"], d) == {"python"} # env > yaml + + +def test_rate_limit_label_reaches_github_branded(monkeypatch): + """End-to-end through act(): a rate_limited ESCALATE artifact must actually + apply a :rate-limited label to the issue. This label is NOT in + allowed_labels, so it must be appended AFTER the allowlist filter (like + escalate_label) — an earlier version wrote it into the artifact only to have + it silently dropped by the filter, so it never reached GitHub. Branded with + bot_name (deequ-bot:rate-limited), not a hardcoded shadow: literal.""" + import json + from unittest import mock + _set_required_env(monkeypatch) + with tempfile.TemporaryDirectory() as tmp: + Path(tmp, ".shadow.yml").write_text("bot:\n name: deequ-bot\n") + monkeypatch.setenv("SHADOW_REPO_ROOT", tmp) + artifact = Path(tmp, "bot_result.json") + artifact.write_text(json.dumps({ + "action": "ESCALATE", "reason": "rate_limited", + "labels": ["needs-human"], "response": "", + "number": 42, "is_pr": True, + "title": "t", "html_url": "https://github.com/o/r/pull/42", + "prompt_id": "n/a", "model_id": "m", + })) + monkeypatch.setenv("ARTIFACT_PATH", str(artifact)) + monkeypatch.setenv("SHADOW_VERIFY_ARTIFACT", "false") # skip HMAC in unit test + + import shadow.main as m + importlib.reload(m) + captured = {} + fake_gh = mock.MagicMock() + fake_gh.add_labels.side_effect = lambda n, labels: captured.setdefault("labels", labels) or True + with mock.patch.object(m, "GitHubClient", return_value=fake_gh), \ + mock.patch.object(m, "SlackClient", return_value=mock.MagicMock()): + m.act() + + assert "deequ-bot:rate-limited" in captured["labels"], captured + assert "needs-human" in captured["labels"] # escalate_label still applied