From 6992b1dd2efa3e718eb72132ea65a65e7bd4f7c0 Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Mon, 28 Sep 2026 02:21:03 -0700 Subject: [PATCH] fix(read): a 404 on a file read lists the nearest existing directory A bare 404 from github_read_file / github_read_pr_file said "repo not found or not accessible, or the path/ref/number doesn't exist". To a model, that reads as an access problem. The QA panel then guessed sibling paths one call at a time: on protoAgent#3771, 26 of its 120 reads were invented filenames. On a 404, probe up to 4 parent directories at the same ref. If one exists, say the repo and ref are fine and only the path is wrong, and list that directory (capped at 60 entries). If none exists, the repo or ref is the problem, so the classified error stands. A non-404 failure skips the probe. Co-Authored-By: Claude Opus 5.5 (1M context) --- PROTO.md | 4 ++ read_tools.py | 43 ++++++++++++++++++++ tests/test_read_tools.py | 85 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+) diff --git a/PROTO.md b/PROTO.md index 83342ed..e1e6a8c 100644 --- a/PROTO.md +++ b/PROTO.md @@ -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): ` 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, diff --git a/read_tools.py b/read_tools.py index 8e2d6eb..793b2e6 100644 --- a/read_tools.py +++ b/read_tools.py @@ -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, @@ -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] @@ -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 diff --git a/tests/test_read_tools.py b/tests/test_read_tools.py index bf111af..9f4ab18 100644 --- a/tests/test_read_tools.py +++ b/tests/test_read_tools.py @@ -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