From 638dba9d1474d41657d2b84002c8deef0ec5baad Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 17 May 2026 21:55:52 +0000 Subject: [PATCH] =?UTF-8?q?fix(workflows):=20bugbot=20=E2=80=94=20gate=20A?= =?UTF-8?q?gent=20Shin=20--close=20on=20'=3D=20true'=20not=20'!=3D=20false?= =?UTF-8?q?'?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PR and issue Agent Shin workflows gated the destructive --close flag with [ "${DISPATCH_CLOSE:-false}" != "false" ]. That pattern treats anything other than the literal string "false" as enabling closure — "True", "yes", "1", typos, accidental whitespace, etc. The workflow_dispatch input UI is a 'true'/'false' choice dropdown so the form is constrained, but the API (`gh workflow run -f close=...`) accepts any string, and a CI cron / external invoker passing a non-canonical truthy value would have silently enabled real contributor PR closures. Mirror the sibling Greptile closer's [ "${CLOSE_FLAG}" = "true" ] pattern: only the EXACT string "true" enables --close; every other value (including the unset/empty default) resolves to dry-run. This is the fail-safe philosophy applied everywhere else in this PR. Added tests/test_litellm/test_github_triage_workflows.py with two parametrized invariants: 1. The destructive gate uses '= "true"' for its env-var comparison (either bare '${ENV}' or '${ENV:-false}' form accepted), and never the fail-open '!= "false"' pattern. 2. Every destructive gate is also gated on AGENT_SHIN_ENABLED being "true" — either by entering the close branch on '=' or by bailing out early on '!=' — so flipping the repo variable off is a true kill switch regardless of per-run inputs. Manually verified the test fails on the buggy '!= "false"' pattern and passes on the fix, so it would have caught the regression at PR time. Co-authored-by: Mateo Wang --- .github/workflows/triage_issue_with_llm.yml | 11 +- .github/workflows/triage_pr_with_llm.yml | 11 +- .../test_github_triage_workflows.py | 135 ++++++++++++++++++ 3 files changed, 153 insertions(+), 4 deletions(-) create mode 100644 tests/test_litellm/test_github_triage_workflows.py diff --git a/.github/workflows/triage_issue_with_llm.yml b/.github/workflows/triage_issue_with_llm.yml index 1aab2cfd533..ff0497f9893 100644 --- a/.github/workflows/triage_issue_with_llm.yml +++ b/.github/workflows/triage_issue_with_llm.yml @@ -58,11 +58,18 @@ jobs: run: | set -euo pipefail ARGS=(--repo "${{ github.repository }}" --issue "${ISSUE_NUMBER}") - if [ "${AGENT_SHIN_ENABLED:-false}" = "true" ] && [ "${DISPATCH_CLOSE:-false}" != "false" ]; then + # Fail-safe gating: only the EXACT string "true" enables the + # destructive --close path. The workflow_dispatch input is a + # `choice` dropdown of "true"/"false" so the UI is constrained, + # but the API (`gh workflow run -f close=...`) accepts any + # string, and a `!= "false"` check would treat "True", "yes", + # "1", "TRUE", typos, and accidental whitespace as enabling + # closure. Mirror the Greptile closer's `= "true"` pattern. + if [ "${AGENT_SHIN_ENABLED:-false}" = "true" ] && [ "${DISPATCH_CLOSE:-false}" = "true" ]; then ARGS+=(--close) echo "::notice::Agent Shin is ENABLED and running in close-on-fail mode." elif [ "${AGENT_SHIN_ENABLED:-false}" = "true" ]; then - echo "::notice::Agent Shin is ENABLED but this trigger is dry-run." + echo "::notice::Agent Shin is ENABLED but this trigger is dry-run (workflow_dispatch close != 'true')." else echo "::notice::Agent Shin is in DRY-RUN mode (AGENT_SHIN_ENABLED is not 'true'). No comments will be posted; no issues will be closed." fi diff --git a/.github/workflows/triage_pr_with_llm.yml b/.github/workflows/triage_pr_with_llm.yml index 58d2aaee66c..eac7e6a56b3 100644 --- a/.github/workflows/triage_pr_with_llm.yml +++ b/.github/workflows/triage_pr_with_llm.yml @@ -69,11 +69,18 @@ jobs: run: | set -euo pipefail ARGS=(--repo "${{ github.repository }}" --pr "${PR_NUMBER}") - if [ "${AGENT_SHIN_ENABLED:-false}" = "true" ] && [ "${DISPATCH_CLOSE:-false}" != "false" ]; then + # Fail-safe gating: only the EXACT string "true" enables the + # destructive --close path. The workflow_dispatch input is a + # `choice` dropdown of "true"/"false" so the UI is constrained, + # but the API (`gh workflow run -f close=...`) accepts any + # string, and a `!= "false"` check would treat "True", "yes", + # "1", "TRUE", typos, and accidental whitespace as enabling + # closure. Mirror the Greptile closer's `= "true"` pattern. + if [ "${AGENT_SHIN_ENABLED:-false}" = "true" ] && [ "${DISPATCH_CLOSE:-false}" = "true" ]; then ARGS+=(--close) echo "::notice::Agent Shin is ENABLED and running in close-on-fail mode." elif [ "${AGENT_SHIN_ENABLED:-false}" = "true" ]; then - echo "::notice::Agent Shin is ENABLED but this trigger is dry-run (workflow_dispatch close=false or scheduled event)." + echo "::notice::Agent Shin is ENABLED but this trigger is dry-run (workflow_dispatch close != 'true' or scheduled event)." else echo "::notice::Agent Shin is in DRY-RUN mode (AGENT_SHIN_ENABLED is not 'true'). No comments will be posted; no PRs will be closed." fi diff --git a/tests/test_litellm/test_github_triage_workflows.py b/tests/test_litellm/test_github_triage_workflows.py new file mode 100644 index 00000000000..438b92b6add --- /dev/null +++ b/tests/test_litellm/test_github_triage_workflows.py @@ -0,0 +1,135 @@ +"""Static guardrails for the Agent Shin + Greptile workflow YAML files. + +These workflows can post comments and close PRs/issues on +BerriAI/litellm, so the gating logic that decides "is this a real +close-on-fail run?" must fail-safe on any unexpected input. The risk +is mostly maintenance: someone edits the bash gate, drops a quote, +inverts a comparison, or uses `!= "false"` (which treats "True", +"yes", "1", and typos as enabling closure) and the regression isn't +caught until a real OSS contributor's PR gets auto-closed. + +The tests below pin two invariants across every workflow that gates a +destructive `--close`: + + 1. The gate uses the fail-safe `= "true"` comparison — not `!= "false"`, + not `!= ""`. Only the literal string "true" should ever enable + closure. + 2. The gate also requires `AGENT_SHIN_ENABLED = "true"` (or the + scheduled-job equivalent) — disabling the variable must always + force dry-run. + +Static parsing of the YAML + bash text is the right level of test here: +the gating logic lives in a `run:` block, not in a Python module we can +import, and end-to-end testing a GitHub Actions workflow from CI is +infeasible. A YAML-level guardrail is exactly what would have caught +the original `!= "false"` regression at PR time. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +import yaml + +REPO_ROOT = Path(__file__).resolve().parents[2] +WORKFLOWS_DIR = REPO_ROOT / ".github" / "workflows" + +# Map of workflow file -> the env var name that drives the destructive +# gate inside that workflow's `run:` block. Keeping this table explicit +# (rather than scraping every workflow file) means a new workflow file +# that bypasses the dry-run gating doesn't silently slip past this test. +DESTRUCTIVE_GATE_ENV: dict[str, str] = { + "triage_pr_with_llm.yml": "DISPATCH_CLOSE", + "triage_issue_with_llm.yml": "DISPATCH_CLOSE", + "close_low_quality_prs.yml": "CLOSE_FLAG", +} + + +def _load_workflow(name: str) -> dict: + return yaml.safe_load((WORKFLOWS_DIR / name).read_text()) + + +def _all_run_blocks(workflow: dict) -> list[str]: + """Return every `run:` step's command text, joined.""" + commands: list[str] = [] + jobs = workflow.get("jobs") or {} + for job in jobs.values(): + for step in job.get("steps", []) or []: + if not isinstance(step, dict): + continue + run = step.get("run") + if isinstance(run, str): + commands.append(run) + return commands + + +@pytest.mark.parametrize("workflow_file,env_var", sorted(DESTRUCTIVE_GATE_ENV.items())) +def test_should_use_failsafe_equals_true_comparison( + workflow_file: str, env_var: str +) -> None: + """The destructive `--close` gate must use `= "true"` (fail-safe), not + `!= "false"` (which would treat "True", "yes", "1", or any typo as + enabling closure). + + Both bare `${ENV_VAR}` and `${ENV_VAR:-false}` (with a default) are + accepted forms — what matters is the comparison operator. The + Greptile closer relies on an outer `AGENT_SHIN_ENABLED` gate so it + can use the bare form; the Agent Shin workflows include `:-false` + for defense in depth. Either is fine. + """ + workflow = _load_workflow(workflow_file) + text = "\n".join(_all_run_blocks(workflow)) + assert env_var in text, ( + f"{workflow_file} no longer references {env_var}; was the " + "gating env var renamed without updating this test?" + ) + accepted_patterns = ( + f'"${{{env_var}}}" = "true"', + f'"${{{env_var}:-false}}" = "true"', + ) + assert any(p in text for p in accepted_patterns), ( + f"{workflow_file} must gate the destructive --close flag on the " + f'EXACT string "true" (one of: {accepted_patterns!r}). Mirror ' + 'the Greptile closer pattern; do NOT use `!= "false"` which ' + 'fail-opens on unknown values like "True", "yes", "1", or typos.' + ) + forbidden_patterns = ( + f'"${{{env_var}}}" != "false"', + f'"${{{env_var}:-false}}" != "false"', + f'"${{{env_var}:-true}}" != "false"', + ) + for forbidden in forbidden_patterns: + assert forbidden not in text, ( + f"{workflow_file} uses the fail-open pattern {forbidden!r}. " + 'Switch to `= "true"` so unknown values stay dry-run.' + ) + + +@pytest.mark.parametrize("workflow_file", sorted(DESTRUCTIVE_GATE_ENV)) +def test_should_require_agent_shin_enabled_for_close(workflow_file: str) -> None: + """Every destructive gate must also gate on the global enablement + variable, so flipping `AGENT_SHIN_ENABLED` off is a kill switch + regardless of any per-run input. + + Two patterns are equally fine: + - Positive: `[ "${AGENT_SHIN_ENABLED:-false}" = "true" ]` to enter + the close branch (Agent Shin workflows). + - Negative: `[ "${AGENT_SHIN_ENABLED:-false}" != "true" ]` then + bail out / force dry-run (Greptile closer). + + What matters is that the comparison value is the literal "true"; + `!= "false"` or `= "1"` etc. would not be a true kill switch. + """ + workflow = _load_workflow(workflow_file) + text = "\n".join(_all_run_blocks(workflow)) + accepted_patterns = ( + '"${AGENT_SHIN_ENABLED:-false}" = "true"', + '"${AGENT_SHIN_ENABLED:-false}" != "true"', + ) + assert any(p in text for p in accepted_patterns), ( + f"{workflow_file} must gate destructive actions on " + '`AGENT_SHIN_ENABLED = "true"` (or the inverted `!= "true"` ' + "guard that forces dry-run). Without this, an unset repo " + "variable would not be treated as a kill switch." + )