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
4 changes: 4 additions & 0 deletions PROTO.md
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,10 @@ branched on `token_source()` (a rejected env/config token says "replace/unset it
named when known); 403/429 + "rate limit" β‡’ `Error: GitHub API rate limit hit β€” retry …`;
binary missing β‡’ `Error: gh CLI is not installed or not on PATH (looked in …)`. Anything
else keeps the `Error (gh exit N): <stderr>` shape. Add a category here, not in a tool.
The one tool-side refinement: a 404 from `github_read_file` / `github_read_pr_file` probes
up to 4 parent directories at the same ref (`read_tools._missing_path_hint`). If one
exists, the error says the PATH is wrong and lists that directory, so the model stops
guessing sibling paths. If none exists, the classified 404 above stands.

**First-run status** (`status.py`): `GET /api/plugins/github/status` β†’ `{gh_path,
gh_version, authenticated, login, host, token_source, error, default_repo, repos,
Expand Down
43 changes: 43 additions & 0 deletions read_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,45 @@ def tail(shown_to: int) -> str:
return True, body + ("" if body.endswith("\n") else "\n") + marker


_MAX_HINT_ENTRIES = 60 # directory entries listed in a missing-path hint
_MAX_HINT_PROBES = 4 # parent directories probed before giving up (a 404 costs a call each)


async def _missing_path_hint(repo: str, path: str, ref: str) -> str | None:
"""For a contents read that 404'd: the nearest EXISTING parent directory and its
entries, or None when no probed parent exists (then the repo or ref is the problem).

A bare 404 reads to a model as "repo not accessible", so it guesses sibling paths one
call at a time (a review burned 20+ reads on invented filenames). Listing the
directory that does exist turns that into one pick."""
parts = [p for p in path.strip().strip("/").split("/") if p]
parents = ["/".join(parts[:i]) for i in range(len(parts) - 1, -1, -1)][:_MAX_HINT_PROBES]
for parent in parents:
args = ["api", "--method", "GET", f"repos/{repo}/contents/{parent}" if parent else f"repos/{repo}/contents"]
if ref.strip():
args += ["-f", f"ref={ref.strip()}"]
rc, out, serr = await run_gh(args)
if rc != 0:
if error_kind(rc, serr or out) != "not_found":
return None # auth / rate limit β€” not evidence about the path
continue
items, perr = parse_json(out, (list,))
if perr:
return None
entries = sorted(
f"{e.get('name', '?')}/" if e.get("type") == "dir" else str(e.get("name", "?")) for e in dicts(items)
)
shown = ", ".join(entries[:_MAX_HINT_ENTRIES])
more = f", … (+{len(entries) - _MAX_HINT_ENTRIES} more)" if len(entries) > _MAX_HINT_ENTRIES else ""
at = f" at {ref.strip()[:12]}" if ref.strip() else ""
return (
f"Error: '{path}' does not exist in {repo}{at} β€” the repo and ref are fine, the PATH is wrong. "
f"The nearest existing directory '{parent or '.'}' contains: {shown}{more}. "
"Pick from these (or list a subdirectory with github_repo_contents) instead of guessing another path."
)
return None


# statusCheckRollup carries two shapes (verified against gh 2.92): a CheckRun
# {name, status: COMPLETED|IN_PROGRESS|QUEUED|…, conclusion: SUCCESS|FAILURE|SKIPPED|
# CANCELLED|NEUTRAL|TIMED_OUT|ACTION_REQUIRED|""} and a StatusContext {context,
Expand Down Expand Up @@ -509,6 +548,8 @@ async def github_read_file(path: str, repo: str = "", ref: str = "", start_line:
args += ["-f", f"ref={ref}"]
rc, out, serr = await run_gh(args)
if gh_err := check_gh_error(rc, serr, repo=repo):
if error_kind(rc, serr) == "not_found" and (hint := await _missing_path_hint(repo, path, ref)):
return hint
return gh_err
# The contents API always returns the WHOLE file; the range is sliced here.
return _render_file(out, start_line, end_line, path=path)[1]
Expand Down Expand Up @@ -568,6 +609,8 @@ async def github_read_pr_file(
if gh_err := check_gh_error(rc, serr, repo=repo):
# Fail LOUD, never fall back to the default branch: a silent fallback is
# exactly the bug this tool exists to prevent.
if error_kind(rc, serr) == "not_found" and (hint := await _missing_path_hint(repo, path, head)):
gh_err = hint
return f"Error reading {path} at {repo}#{number} head {head[:12]}: {gh_err}"
ok, out = _render_file(out, start_line, end_line, path=f"{path} at {repo}#{number} head {head[:12]}")
if not ok: # a start_line past EOF β€” the error alone, no content header over it
Expand Down
85 changes: 85 additions & 0 deletions tests/test_read_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -1113,3 +1113,88 @@ def test_render_invariants_over_random_files():
assert (int(m[1]), int(m[2]), int(m[3])) == (start, total, len(line)) and len(line) > cap
assert shown == line[:cap]
assert (m[6] is None) == (start >= total)


# ── a 404 on a contents read names the nearest existing directory ───────────────

_NOT_FOUND = (1, "", "gh: Not Found (HTTP 404)")
_SRC_LISTING = json.dumps(
[
{"name": "ThemeQuickButton.tsx", "type": "file"},
{"name": "goals", "type": "dir"},
]
)


def _tree_gh(existing: dict[str, str], calls: list):
"""A fake `gh`: contents reads succeed only for the directories in ``existing``."""

async def fake_gh(args, **kw):
calls.append(args)
endpoint = next(a for a in args if a.startswith("repos/"))
if "/pulls/" in endpoint:
return 0, "b" * 40 + "\n", ""
path = endpoint.split("/contents", 1)[1].lstrip("/")
if path in existing:
return 0, existing[path], ""
return _NOT_FOUND

return fake_gh


@pytest.mark.asyncio
async def test_read_file_404_lists_the_nearest_existing_directory():
calls: list = []
fake = _tree_gh({"apps/web/src": _SRC_LISTING}, calls)
with patch("ghplugin.read_tools.run_gh", new=AsyncMock(side_effect=fake)):
out = await _read_file_tool().ainvoke(
{"repo": "o/r", "path": "apps/web/src/settings/ThemeQuickDialog.tsx", "ref": "abc123"}
)
assert out.startswith("Error: 'apps/web/src/settings/ThemeQuickDialog.tsx' does not exist in o/r at abc123")
assert "the PATH is wrong" in out
assert "'apps/web/src' contains: ThemeQuickButton.tsx, goals/" in out
# every probe is a GET pinned to the same ref β€” a POST would 404 regardless (#118)
for args in calls:
assert args[:3] == ["api", "--method", "GET"]
assert "ref=abc123" in args


@pytest.mark.asyncio
async def test_read_pr_file_404_lists_the_directory_at_the_pr_head():
calls: list = []
fake = _tree_gh({"apps/web/src": _SRC_LISTING}, calls)
with patch("ghplugin.read_tools.run_gh", new=AsyncMock(side_effect=fake)):
out = await _read_pr_file_tool().ainvoke(
{"repo": "o/r", "number": 3771, "path": "apps/web/src/goals/GoalCreateDialog.tsx"}
)
assert out.startswith("Error reading apps/web/src/goals/GoalCreateDialog.tsx at o/r#3771 head bbbbbbbbbbbb:")
assert "contains: ThemeQuickButton.tsx, goals/" in out
assert all(f"ref={'b' * 40}" in a for a in calls[1:])


@pytest.mark.asyncio
async def test_404_with_no_existing_parent_keeps_the_classified_error():
# the ref (or repo) is what's wrong β€” no directory to point at, so the old message stands
calls: list = []
with patch("ghplugin.read_tools.run_gh", new=AsyncMock(side_effect=_tree_gh({}, calls))):
out = await _read_file_tool().ainvoke({"repo": "o/r", "path": "a/b/c/d/e/f.py", "ref": "nope"})
assert out.startswith("Error: repo 'o/r' not found or not accessible, or the path/ref/number doesn't exist")
assert len(calls) == 1 + 4 # the read + the capped parent probes


@pytest.mark.asyncio
async def test_non_404_failure_skips_the_directory_probe():
mock = AsyncMock(return_value=(1, "", "API rate limit exceeded (HTTP 403)"))
with patch("ghplugin.read_tools.run_gh", mock):
out = await _read_file_tool().ainvoke({"repo": "o/r", "path": "x/y.py"})
assert "rate limit" in out
assert mock.call_count == 1


@pytest.mark.asyncio
async def test_missing_path_hint_caps_the_listing():
listing = json.dumps([{"name": f"f{i:03d}.py", "type": "file"} for i in range(75)])
with patch("ghplugin.read_tools.run_gh", new=AsyncMock(side_effect=_tree_gh({"": listing}, []))):
out = await _read_file_tool().ainvoke({"repo": "o/r", "path": "nope.py"})
assert "'.' contains: f000.py" in out and "f059.py" in out and "f060.py" not in out
assert "(+15 more)" in out
Loading