diff --git a/scripts/codex_warden_hooks.py b/scripts/codex_warden_hooks.py index 25600c9d..d4bad698 100644 --- a/scripts/codex_warden_hooks.py +++ b/scripts/codex_warden_hooks.py @@ -40,6 +40,7 @@ from warden_hooks.command_parse import ( # noqa: E402 _command_hash, _extract_pr_ref, + _extract_repo_flag, _gh_command_index_after_global_flags, _is_admin_merge_command, _is_gh_executable, @@ -866,7 +867,8 @@ def approve_admin_merge(command: str, repo_root: Path) -> int: pr_ref = _extract_pr_ref(command) # Current-branch merges (no ref) pass ``gh pr checks`` the branch name check_ref = pr_ref or "HEAD" - status, detail = _check_ci_status(check_ref) + repo = _extract_repo_flag(command) + status, detail = _check_ci_status(check_ref, repo=repo) block = _ci_block_reason(check_ref, status, detail) if block: print(block, file=sys.stderr) @@ -1700,15 +1702,23 @@ def _verdict_in(verdicts: dict[str, Any], marker_name: str) -> str | None: return None -def _gh_pr_head_branch(ref: str, timeout: int = 3) -> str | None: +def _gh_pr_head_branch( + ref: str, timeout: int = 3, repo: str | None = None +) -> str | None: """Resolve a PR ref (number or URL) to its head branch via ``gh pr view``. Returns None on any failure so the caller fails safe (treats it as an unverifiable match and falls through to the one-shot approval path). + + *repo* scopes the lookup to an explicit ``OWNER/REPO`` instead of ``gh``'s + cwd-based resolution — see ``_query_gh_checks`` for why this matters. """ + argv = ["gh", "pr", "view", ref, "--json", "headRefName"] + if repo: + argv.extend(["--repo", repo]) try: result = subprocess.run( - ["gh", "pr", "view", ref, "--json", "headRefName"], + argv, text=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, @@ -1742,7 +1752,8 @@ def _pr_matches_worktree(command: str, wt: Path) -> tuple[bool, str]: ref = _extract_pr_ref(command) if ref is None or ref == wt_branch: return (True, "") - head = _gh_pr_head_branch(ref) + repo = _extract_repo_flag(command) + head = _gh_pr_head_branch(ref, repo=repo) if head is None: return ( False, @@ -1842,7 +1853,8 @@ def run_admin_merge_gate(event: dict[str, Any], repo_root: Path) -> int: pr_ref = _extract_pr_ref(command) check_ref = pr_ref or "HEAD" - ci_status, ci_detail = _check_ci_status(check_ref) + repo = _extract_repo_flag(command) + ci_status, ci_detail = _check_ci_status(check_ref, repo=repo) ci_block = _ci_block_reason(check_ref, ci_status, ci_detail) if ci_block: _block_pre_tool(ci_block) diff --git a/scripts/tests/test_codex_warden_hooks.py b/scripts/tests/test_codex_warden_hooks.py index 76fa1f7f..51ff42bf 100644 --- a/scripts/tests/test_codex_warden_hooks.py +++ b/scripts/tests/test_codex_warden_hooks.py @@ -2083,6 +2083,61 @@ def fake_run(cmd, *args, **kwargs): ] +def test_check_ci_status_no_repo_argv_unchanged(monkeypatch): + # Backward-compat guard: omitting `repo` must produce byte-identical argv + # to today (no trailing --repo), independent of the assertion above. + hooks = load_hooks() + captured = {} + + def fake_run(cmd, *args, **kwargs): + if ( + isinstance(cmd, (list, tuple)) + and len(cmd) >= 3 + and str(cmd[0]).endswith("gh") + and cmd[1] == "pr" + and cmd[2] == "checks" + ): + captured["cmd"] = list(cmd) + return subprocess.CompletedProcess( + cmd, 0, stdout=json.dumps([{"bucket": "pass", "name": "ci"}]), stderr="" + ) + return _REAL_SUBPROCESS_RUN(cmd, *args, **kwargs) + + monkeypatch.setattr(hooks.subprocess, "run", fake_run) + hooks._check_ci_status("123") + assert "--repo" not in captured["cmd"] + + +def test_check_ci_status_explicit_repo_scopes_gh_call(monkeypatch): + # Fixes the confirmed cross-repo bug: when the gated command carries an + # explicit repo, the gate's own internal gh call must be scoped to it, + # not to whatever repo the cwd's git remote happens to resolve. + hooks = load_hooks() + captured = {} + + def fake_run(cmd, *args, **kwargs): + if ( + isinstance(cmd, (list, tuple)) + and len(cmd) >= 3 + and str(cmd[0]).endswith("gh") + and cmd[1] == "pr" + and cmd[2] == "checks" + ): + captured["cmd"] = list(cmd) + return subprocess.CompletedProcess( + cmd, 0, stdout=json.dumps([{"bucket": "pass", "name": "ci"}]), stderr="" + ) + return _REAL_SUBPROCESS_RUN(cmd, *args, **kwargs) + + monkeypatch.setattr(hooks.subprocess, "run", fake_run) + status, _ = hooks._check_ci_status("14", repo="owner/other-repo") + assert status == hooks._CI_STATUS_GREEN + assert captured["cmd"] == [ + "gh", "pr", "checks", "14", "--json", "bucket,name", "--required", + "--repo", "owner/other-repo", + ] + + def test_check_ci_status_advisory_pending_does_not_block(monkeypatch): # Required checks all pass; advisory checks (TrueCourse etc.) still pending in # the unfiltered set. The gate sees only required → GREEN (no block). @@ -2348,6 +2403,77 @@ def test_extract_pr_ref_body_flag_before_positional(): assert hooks._extract_pr_ref('gh pr merge --squash -b "fix: blah" 294') == "294" +# ── _extract_repo_flag ─────────────────────────────────────────────────────── + + +def test_extract_repo_flag_global_position(): + hooks = load_hooks() + assert ( + hooks._extract_repo_flag("gh --repo owner/repo pr merge 294 --admin") + == "owner/repo" + ) + + +def test_extract_repo_flag_short_flag_global_position(): + hooks = load_hooks() + assert ( + hooks._extract_repo_flag("gh -R owner/repo pr merge 294 --admin") + == "owner/repo" + ) + + +def test_extract_repo_flag_short_flag_attached_form(): + # gh's short-flag attached form: `-Rowner/repo` (no space). + hooks = load_hooks() + assert ( + hooks._extract_repo_flag("gh pr merge -Rowner/repo --admin 294") + == "owner/repo" + ) + + +def test_extract_repo_flag_subcommand_local_position(): + # The shape production landing commands actually use: + # `gh pr merge --repo owner/repo --admin --squash `. + hooks = load_hooks() + assert ( + hooks._extract_repo_flag("gh pr merge --repo owner/repo --admin --squash 294") + == "owner/repo" + ) + + +def test_extract_repo_flag_equals_form(): + hooks = load_hooks() + assert ( + hooks._extract_repo_flag("gh pr merge --repo=owner/repo --admin 294") + == "owner/repo" + ) + + +def test_extract_repo_flag_absent_returns_none(): + hooks = load_hooks() + assert hooks._extract_repo_flag("gh pr merge --admin 294") is None + + +def test_extract_repo_flag_duplicate_last_wins(): + # Mirrors gh's own last-flag-wins precedence for repeated flags. + hooks = load_hooks() + assert ( + hooks._extract_repo_flag( + "gh --repo first/repo pr merge --repo second/repo --admin 294" + ) + == "second/repo" + ) + + +def test_extract_repo_flag_does_not_misread_other_flag_values(): + # A body value that happens to look flag-like must not be misread as + # a repo flag -- `_FLAGS_WITH_VALUE` skips it correctly. + hooks = load_hooks() + assert ( + hooks._extract_repo_flag('gh pr merge --admin -b "--repo" 294') is None + ) + + # ── CI gate integration: run_admin_merge_gate ──────────────────────────────── @@ -2371,6 +2497,58 @@ def test_admin_merge_gate_blocks_when_ci_red(monkeypatch, tmp_path, capsys): assert "gh pr checks 294" in reason +def test_admin_merge_gate_scopes_ci_check_to_explicit_repo(monkeypatch, tmp_path, capsys): + # Regression test for the confirmed cross-repo bug: a `gh pr merge --repo + # ` command must be graded against THAT repo's CI, not whatever the + # worktree's own git remote happens to resolve to. Simulate the exact + # failure mode -- CI is genuinely GREEN on the named repo but would read + # RED if the gate ever queried unscoped -- and confirm the gate allows + # (pre-approved) using the scoped query, never the unscoped one. + hooks = load_hooks() + repo = git_repo(tmp_path) + command = "gh pr merge --repo owner/other-repo --admin --squash 14" + captured = {} + + def fake_run(cmd, *args, **kwargs): + if ( + isinstance(cmd, (list, tuple)) + and len(cmd) >= 3 + and str(cmd[0]).endswith("gh") + and cmd[1] == "pr" + and cmd[2] == "checks" + ): + captured["cmd"] = list(cmd) + if "--repo" in cmd: + # Scoped query: this is the real target repo, CI is green. + return subprocess.CompletedProcess( + cmd, 0, stdout=json.dumps([{"bucket": "pass", "name": "ci"}]), stderr="" + ) + # Unscoped query: simulates the bug -- resolves to a DIFFERENT, + # unrelated PR whose CI is red. If the gate ever calls gh without + # --repo here, this branch fires and the test fails loudly below. + return subprocess.CompletedProcess( + cmd, 1, stdout=json.dumps([{"bucket": "fail", "name": "ci"}]), stderr="" + ) + return _REAL_SUBPROCESS_RUN(cmd, *args, **kwargs) + + monkeypatch.setattr(hooks.subprocess, "run", fake_run) + + marker = repo / ".claude" / ".admin-merge-approved" + marker.parent.mkdir(parents=True, exist_ok=True) + marker.write_text( + json.dumps({"command_hash": hooks._command_hash(command), "command": command}), + encoding="utf-8", + ) + + rc = hooks.run_admin_merge_gate(bash_event(repo, command), repo) + + assert rc == 0 + assert not marker.exists() # consumed -- approval matched, no denial + assert "permissionDecision" not in capsys.readouterr().out + assert "--repo" in captured["cmd"] + assert captured["cmd"][captured["cmd"].index("--repo") + 1] == "owner/other-repo" + + def test_admin_merge_gate_blocks_when_ci_pending(monkeypatch, tmp_path, capsys): hooks = load_hooks() repo = git_repo(tmp_path) @@ -4821,6 +4999,72 @@ def test_pr_matches_worktree_no_ref_matches_current_branch(tmp_path): assert matched is True # no explicit ref => current branch => this worktree's PR +def test_gh_pr_head_branch_no_repo_argv_unchanged(monkeypatch): + hooks = load_hooks() + captured = {} + + def fake_run(cmd, *args, **kwargs): + captured["cmd"] = list(cmd) + return subprocess.CompletedProcess( + cmd, 0, stdout=json.dumps({"headRefName": "some-branch"}), stderr="" + ) + + monkeypatch.setattr(hooks.subprocess, "run", fake_run) + head = hooks._gh_pr_head_branch("294") + assert head == "some-branch" + assert captured["cmd"] == ["gh", "pr", "view", "294", "--json", "headRefName"] + + +def test_gh_pr_head_branch_explicit_repo_scopes_gh_call(monkeypatch): + hooks = load_hooks() + captured = {} + + def fake_run(cmd, *args, **kwargs): + captured["cmd"] = list(cmd) + return subprocess.CompletedProcess( + cmd, 0, stdout=json.dumps({"headRefName": "lia-410-branch"}), stderr="" + ) + + monkeypatch.setattr(hooks.subprocess, "run", fake_run) + head = hooks._gh_pr_head_branch("14", repo="owner/other-repo") + assert head == "lia-410-branch" + assert captured["cmd"] == [ + "gh", "pr", "view", "14", "--json", "headRefName", + "--repo", "owner/other-repo", + ] + + +def test_pr_matches_worktree_threads_explicit_repo_to_gh_pr_view(monkeypatch, tmp_path): + # End-to-end: a `gh pr merge --repo o/r ` command must scope the + # PR-head-branch lookup to that same repo, not the worktree's own remote. + hooks = load_hooks() + repo = git_repo(tmp_path) + _commit_repo(repo) + captured = {} + + def fake_run(cmd, *args, **kwargs): + if ( + isinstance(cmd, (list, tuple)) + and len(cmd) >= 3 + and str(cmd[0]).endswith("gh") + and cmd[1] == "pr" + and cmd[2] == "view" + ): + captured["cmd"] = list(cmd) + return subprocess.CompletedProcess( + cmd, 0, stdout=json.dumps({"headRefName": "other-branch"}), stderr="" + ) + return _REAL_SUBPROCESS_RUN(cmd, *args, **kwargs) + + monkeypatch.setattr(hooks.subprocess, "run", fake_run) + matched, _ = hooks._pr_matches_worktree( + "gh pr merge --repo owner/other-repo --admin 999", repo + ) + assert matched is False # head branch differs from this worktree's branch + assert "--repo" in captured["cmd"] + assert captured["cmd"][captured["cmd"].index("--repo") + 1] == "owner/other-repo" + + def test_block_message_diagnoses_bucket_mismatch(tmp_path): """A 'not run yet' model backend whose SHIP sits in a SIBLING bucket gets a pointer to that bucket instead of a silent retry.""" diff --git a/scripts/warden_hooks/ci_status.py b/scripts/warden_hooks/ci_status.py index 36cb711a..caaf337c 100644 --- a/scripts/warden_hooks/ci_status.py +++ b/scripts/warden_hooks/ci_status.py @@ -36,7 +36,7 @@ def _query_gh_checks( - pr_ref: str, *, required_only: bool, timeout: int = 3 + pr_ref: str, *, required_only: bool, timeout: int = 3, repo: str | None = None ) -> tuple[str, str, int]: """Run ``gh pr checks`` once and classify the result. @@ -45,10 +45,20 @@ def _query_gh_checks( When *required_only* is set, the query is scoped with ``--required`` so the gate sees only branch-protection-required checks. Failure to query defaults to ``_CI_STATUS_ERROR`` so the caller blocks rather than falls open. + + *repo* scopes the query to an explicit ``OWNER/REPO`` (via ``gh``'s own + ``--repo`` flag) instead of letting ``gh`` resolve one from the current + working directory's git remote — needed when the gated command already + names an explicit repo different from the caller's own cwd (e.g. a + worktree tree whose ambient remote belongs to a different repo). ``None`` + (the default) keeps today's cwd-based resolution, so the constructed argv + is byte-identical when no explicit repo is given. """ argv = ["gh", "pr", "checks", pr_ref, "--json", "bucket,name"] if required_only: argv.append("--required") + if repo: + argv.extend(["--repo", repo]) try: result = subprocess.run( argv, @@ -115,7 +125,9 @@ def _query_gh_checks( return _CI_STATUS_ERROR, f"unknown check buckets: {', '.join(sorted(unknown))}", n -def _check_ci_status(pr_ref: str, timeout: int = 3) -> tuple[str, str]: +def _check_ci_status( + pr_ref: str, timeout: int = 3, repo: str | None = None +) -> tuple[str, str]: """Classify CI for *pr_ref*, scoped to branch-protection-required checks. The admin-merge gate must mirror branch protection — only checks the repo @@ -126,8 +138,13 @@ def _check_ci_status(pr_ref: str, timeout: int = 3) -> tuple[str, str]: Falls closed: an unverifiable status — or a PR that has checks but none required — blocks rather than allowing an unreviewed admin-merge. + + *repo*: see ``_query_gh_checks`` — scopes both queries below to an + explicit ``OWNER/REPO`` instead of ``gh``'s cwd-based resolution. """ - status, message, _ = _query_gh_checks(pr_ref, required_only=True, timeout=timeout) + status, message, _ = _query_gh_checks( + pr_ref, required_only=True, timeout=timeout, repo=repo + ) if status != _CI_STATUS_NO_CHECKS: return status, message @@ -135,7 +152,7 @@ def _check_ci_status(pr_ref: str, timeout: int = 3) -> tuple[str, str]: # genuinely zero checks → allowed through (unchanged behaviour); checks # present but none required → ambiguous, fail closed. all_status, all_message, all_n = _query_gh_checks( - pr_ref, required_only=False, timeout=timeout + pr_ref, required_only=False, timeout=timeout, repo=repo ) if all_status == _CI_STATUS_ERROR: return all_status, all_message diff --git a/scripts/warden_hooks/command_parse.py b/scripts/warden_hooks/command_parse.py index 3e6038a0..2faa99c4 100644 --- a/scripts/warden_hooks/command_parse.py +++ b/scripts/warden_hooks/command_parse.py @@ -69,16 +69,22 @@ def _is_admin_merge_command(command: str) -> bool: return False +#: ``gh pr merge``/``gh pr view`` flags that consume the following token as +#: their value. Shared by ``_extract_pr_ref`` (to skip over them while hunting +#: for the PR ref) and ``_extract_repo_flag`` (to avoid misreading one of their +#: values as a repo flag). +_FLAGS_WITH_VALUE = frozenset({ + "-R", "--repo", "-t", "--subject-body", + "--match-head-commit", "--author", + "-b", "--body", "-F", "--body-file", "-A", "--author-email", +}) + + def _extract_pr_ref(command: str) -> str | None: """Return the PR number, URL, or branch from a ``gh pr merge`` command. Scans past flags so ``gh pr merge --squash 294`` is handled correctly. """ - _FLAGS_WITH_VALUE = frozenset({ - "-R", "--repo", "-t", "--subject-body", - "--match-head-commit", "--author", - "-b", "--body", "-F", "--body-file", "-A", "--author-email", - }) tokens = _shell_tokens(command) for index, token in enumerate(tokens): if not _is_gh_executable(token): @@ -100,3 +106,47 @@ def _extract_pr_ref(command: str) -> str | None: i += 1 return None return None + + +def _extract_repo_flag(command: str) -> str | None: + """Return an explicit ``-R``/``--repo`` value from a ``gh`` invocation, if any. + + ``gh`` accepts ``-R``/``--repo <[HOST/]OWNER/REPO>`` either as a global flag + (before the subcommand, e.g. ``gh --repo o/r pr merge 294``) or as a + subcommand-local flag (after it, e.g. ``gh pr merge --repo o/r 294`` — the + shape production callers actually use). Scans every token after ``gh`` + uniformly, using ``_FLAGS_WITH_VALUE`` to skip the VALUE of every other + flag so it is never misread as a repo flag. When ``--repo``/``-R`` appears + more than once, the LAST occurrence wins, matching ``gh``'s own + last-flag-wins precedence. Returns ``None`` when no explicit repo scope is + given, so callers fall back to ``gh``'s own cwd-based git-remote + resolution (today's unchanged default behaviour). + """ + tokens = _shell_tokens(command) + for index, token in enumerate(tokens): + if not _is_gh_executable(token): + continue + found: str | None = None + i = index + 1 + while i < len(tokens): + tok = tokens[i] + if tok in ("-R", "--repo"): + if i + 1 < len(tokens): + found = tokens[i + 1] + i += 2 + continue + if tok.startswith("--repo="): + found = tok.split("=", 1)[1] + i += 1 + continue + if tok.startswith("-R") and tok != "-R" and not tok.startswith("--"): + # gh's short-flag attached form: `-Rowner/repo`. + found = tok[2:] + i += 1 + continue + if tok in _FLAGS_WITH_VALUE and i + 1 < len(tokens): + i += 2 + continue + i += 1 + return found + return None