diff --git a/read_tools.py b/read_tools.py index f6ec075..959797c 100644 --- a/read_tools.py +++ b/read_tools.py @@ -378,7 +378,17 @@ async def github_read_file(path: str, repo: str = "", ref: str = "") -> str: repo = resolve_repo(repo, default_repo) or "" if err := bad_repo(repo): return err - args = ["api", f"repos/{repo}/contents/{path}", "-H", "Accept: application/vnd.github.raw+json"] + # `--method GET` is load-bearing: `gh api` switches to POST as soon as a `-f` field is + # added, and POST /contents/{path} is a 404 on GitHub — every ref-pinned read failed + # (pr-reviewer-plugin#118). With GET, gh sends the fields as query params (?ref=…). + args = [ + "api", + "--method", + "GET", + f"repos/{repo}/contents/{path}", + "-H", + "Accept: application/vnd.github.raw+json", + ] if ref.strip(): args += ["-f", f"ref={ref}"] rc, out, serr = await run_gh(args) @@ -418,6 +428,8 @@ async def github_read_pr_file(number: int, path: str, repo: str = "") -> str: rc, out, serr = await run_gh( [ "api", + "--method", + "GET", # a `-f` field makes gh default to POST, a 404 here (#118) f"repos/{repo}/contents/{path}", "-H", "Accept: application/vnd.github.raw+json", @@ -456,7 +468,7 @@ async def github_path_exists(path: str, repo: str = "", ref: str = "") -> str: if not path.strip(): return "Error: `path` is empty." clean = path.strip().strip("/") - args = ["api", f"repos/{repo}/contents/{clean}"] + args = ["api", "--method", "GET", f"repos/{repo}/contents/{clean}"] # GET: see github_read_file if ref.strip(): args += ["-f", f"ref={ref.strip()}"] rc, out, serr = await run_gh(args) @@ -489,7 +501,8 @@ async def github_repo_contents(repo: str = "", path: str = "", ref: str = "") -> repo = resolve_repo(repo, default_repo) or "" if err := bad_repo(repo): return err - args = ["api", f"repos/{repo}/contents/{path}" if path else f"repos/{repo}/contents"] + # GET explicitly — a `-f ref=` field would otherwise make gh send POST (see github_read_file). + args = ["api", "--method", "GET", f"repos/{repo}/contents/{path}" if path else f"repos/{repo}/contents"] if ref.strip(): args += ["-f", f"ref={ref}"] rc, out, serr = await run_gh(args) diff --git a/tests/test_read_tools.py b/tests/test_read_tools.py index 4987bbc..8596e93 100644 --- a/tests/test_read_tools.py +++ b/tests/test_read_tools.py @@ -105,7 +105,7 @@ async def test_repo_contents_root_passes_no_trailing_slash(): mock = AsyncMock(return_value=(0, _CONTENTS_JSON, "")) with patch("ghplugin.read_tools.run_gh", mock): await _repo_contents_tool().ainvoke({"repo": "owner/name"}) - assert mock.call_args.args[0] == ["api", "repos/owner/name/contents"] + assert mock.call_args.args[0] == ["api", "--method", "GET", "repos/owner/name/contents"] @pytest.mark.asyncio @@ -779,3 +779,81 @@ def test_search_issues_description_says_dedupe_before_filing(): "github_create_issue" ].description ) + + +# ── Every contents read must be a GET (pr-reviewer-plugin#118) ─────────────────── +# `gh api` picks the HTTP method itself: "The default HTTP request method is GET +# normally and POST if any parameters were added. Override the method with +# --method" (`gh api --help`). Passing the ref as `-f ref=` without `--method GET` +# therefore sent `POST /repos/{o}/{r}/contents/{path}`, which GitHub answers with +# 404 Not Found — on files that exist. In production that made EVERY ref-pinned +# `github_read_file` and EVERY `github_read_pr_file` call fail (0 successes across +# ~2 months of review-panel audit logs), so the panel's lanes reviewed diffs blind. +# The old `"-f" in args` / `"ref=main" in args` assertions stayed green throughout: +# they checked that the ref was PASSED, never how gh would SEND it. +_FIELD_FLAGS = {"-f", "--raw-field", "-F", "--field", "--input"} + + +def _gh_api_method(argv: list[str]) -> str: + """The HTTP method `gh api ` sends, per gh's documented selection rule.""" + for i, arg in enumerate(argv): + if arg in ("-X", "--method") and i + 1 < len(argv): + return argv[i + 1].upper() + if arg.startswith("--method="): + return arg.split("=", 1)[1].upper() + if arg.startswith("-X") and len(arg) > 2: + return arg[2:].upper() + has_fields = any(a in _FIELD_FLAGS or a.startswith(("--field=", "--raw-field=")) for a in argv) + return "POST" if has_fields else "GET" + + +def test_gh_api_method_model_matches_gh_semantics(): + # The rule the regression tests below lean on (checked against gh 2.92 --verbose). + assert _gh_api_method(["api", "repos/o/n/contents/x"]) == "GET" + assert _gh_api_method(["api", "repos/o/n/contents/x", "-f", "ref=main"]) == "POST" + assert _gh_api_method(["api", "--method", "GET", "repos/o/n/contents/x", "-f", "ref=main"]) == "GET" + assert _gh_api_method(["api", "repos/o/n/pulls/1/reviews", "-X", "POST", "-f", "body=x"]) == "POST" + + +def _tool(name: str): + for t in get_read_tools(): + if t.name == name: + return t + raise AssertionError(f"{name} tool not found") + + +_REF = "0123456789abcdef0123456789abcdef01234567" + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("name", "args", "gh_replies"), + [ + ("github_read_file", {"repo": "o/n", "path": "src/a.py", "ref": _REF}, [(0, "body", "")]), + ("github_read_file", {"repo": "o/n", "path": "src/a.py", "ref": "epic/fleet-deck"}, [(0, "body", "")]), + ("github_path_exists", {"repo": "o/n", "path": "src/a.py", "ref": "main"}, [(0, "{}", "")]), + ("github_repo_contents", {"repo": "o/n", "path": "src", "ref": "main"}, [(0, "[]", "")]), + # read_pr_file: resolve the head SHA, then read the file AT that SHA. + ("github_read_pr_file", {"repo": "o/n", "number": 7, "path": "src/a.py"}, [(0, _REF, ""), (0, "body", "")]), + ], +) +async def test_ref_pinned_contents_reads_are_GETs(name, args, gh_replies): + mock = AsyncMock(side_effect=gh_replies) + with patch("ghplugin.read_tools.run_gh", mock): + await _tool(name).ainvoke(args) + contents_calls = [c.args[0] for c in mock.call_args_list if any("/contents" in a for a in c.args[0])] + assert contents_calls, "the tool never read the contents API" + for argv in contents_calls: + assert _gh_api_method(argv) == "GET", f"{name} sends {_gh_api_method(argv)} (404s on GitHub): {argv}" + # …and the ref still travels (as a query parameter on the GET). + want = args.get("ref") or _REF + assert f"ref={want}" in argv, argv + + +@pytest.mark.asyncio +async def test_read_pr_file_reads_at_the_resolved_head(): + mock = AsyncMock(side_effect=[(0, _REF, ""), (0, "print('hi')", "")]) + with patch("ghplugin.read_tools.run_gh", mock): + out = await _tool("github_read_pr_file").ainvoke({"repo": "o/n", "number": 7, "path": "src/a.py"}) + assert out.startswith(f"src/a.py @ o/n#7 head {_REF[:12]}:") + assert "print('hi')" in out