feat: Implement github_read_file in read_tools.py + test#1
Merged
Conversation
|
👀 Quinn is reviewing — verdict (PASS / WARN / FAIL) + findings to follow. |
There was a problem hiding this comment.
QA Audit — PR #1 | feat: Implement github_read_file in read_tools.py + test
VERDICT: WARN (CI pending — re-review on terminal)
CI Status
- test: ⏳ queued
Diff Review
read_tools.py: Stub replaced with realgh apicall (7 lines). Uses existingrun_gh,check_gh_error,bad_repo— consistent with the rest of the file.Accept: application/vnd.github.raw+jsonheader correctly fetches raw content. 20k-char truncation guard is reasonable.tests/test_read_tools.py: 6 clean async tests (68 lines). Host-free, properly mocked withAsyncMockat the correct patch path. Covers success, ref forwarding, bad-repo short-circuit, gh error surfacing, truncation boundary, and empty content.
Observations
- No blocking issues found. Implementation is straightforward and follows established conventions.
- LOW: Truncation message uses Unicode
…— will render fine in any modern terminal/consumer, but worth noting if any downstream ASCII-only pipeline exists.
— Quinn, QA Engineer
|
Submitted COMMENT review on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I'll start by reading the key files to understand the codebase patterns before making changes.I'll start by reading the key files to understand the codebase patterns before making changes.Now let me read
gh_cli.pyand the test conftest to understand the helpers and test patterns.I have everything I need. Now I'll implement thegithub_read_filebody, replacing only the stub return after thebad_repocheck.Now the test file. Let me writetests/test_read_tools.pyfollowing the repo's host-free conventions.Let me verify the lint and tests pass locally, if the environment allows it.The langchain import failure is just the local Python 3.9 env lacking deps (CI installs them). But the ruff import-sort issue is real — let me see what ruff wants.Ruff groupsghpluginas third-party. Let me remove that blank line.Ruff passes. Now let me try to actually run the tests in a venv with the deps, to confirm correctness before this goes to CI.All 12 tests pass (6 new + 4 register + 2 version). Let me clean up the temp venv.Done. Here's what I changed:read_tools.py— implementedgithub_read_fileReplaced the stub body (after the
bad_repo(repo)check, which stays) with a realgh apicall:Nothing else in the file was touched — the docstring, the other seven tools, and the return list are unchanged.
tests/test_read_tools.py— new test fileHost-free, mirroring
test_register.py/test_version.pyconventions: importsget_read_toolsfromghplugin.read_tools, patchesghplugin.read_tools.run_ghwithAsyncMock, and invokes the langchain@toolvia.ainvoke(...). Six cases:"file content"-fandref=main"bad"→ starts with"Error: 'repo' must be"(and assertsrun_ghis never called)(1, "", "not found")→ starts with"Error (gh exit 1)""… (truncated at 20000 chars)", exact length asserted""Verification
ruff check . && ruff format --check .→ All checks passedpytest -q(in a Python 3.12 venv withrequirements-dev.txt) → 12 passed — the 6 new tests plus the existingtest_register.pyandtest_version.pyall green.One note: the task spec estimated the truncated length as "~20032 chars"; the actual value is 20029 (
20000 + len("\n… (truncated at 20000 chars)"), with…being a single Unicode char). My test asserts that exact, self-consistent length rather than the rounded estimate, so it can't drift from the implementation.