feat: Implement CLI functionality for user query processing#15
Open
programmer-ke wants to merge 1 commit into
Open
feat: Implement CLI functionality for user query processing#15programmer-ke wants to merge 1 commit into
programmer-ke wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a vendored dot-notation config loader, threads a ChangesConfig-driven multi-repo CLI with branch-aware sync
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as __main__.py
participant Config as dotconfig.Config
participant MessageBus as bootstrap/handlers
participant RepoStorage as FileSystemRepoStorage
participant Whoosh as WhooshDocumentIndex
participant Agent as OpenAI research agent
User->>CLI: run with --update-sources
CLI->>Config: _find_config / _load_config
Config-->>CLI: repositories, branches, extensions
loop for each configured repo
CLI->>MessageBus: SyncRepo(repo_url, branch)
MessageBus->>RepoStorage: clone_repo(url, branch) or pull_repo(branch)
RepoStorage-->>MessageBus: RepositorySynced(url, branch)
end
User->>CLI: run with --query
CLI->>Config: _load_config
loop for each configured repo
CLI->>Whoosh: WhooshDocumentIndex(index_location, file_prefix)
CLI->>CLI: make_search_tool(index, tool_id, content_information)
end
CLI->>Agent: make_openai_research_agent(prompt, model_name, base_url)
CLI->>Agent: find_answer(query, tools)
Agent-->>CLI: answer with references
CLI-->>User: print answer and reference paths
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/docs_buddy/adapters/__init__.py (1)
139-169: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHarden git commands against branch names starting with
-.If a branch name begins with
-, git may interpret it as an option flag rather than a value. Using=syntax for--branchand--separators for positional args prevents this.🛡️ Proposed fix
`@staticmethod` def _clone_repository(url: str, target_dir: str, branch: str) -> None: """Clone a git repository with depth 1.""" subprocess.run( - ["git", "clone", "--depth", "1", "--branch", branch, url, target_dir], + ["git", "clone", "--depth", "1", f"--branch={branch}", "--", url, target_dir], check=True, capture_output=True, text=True, ) `@staticmethod` def _update_repository(directory: str, branch: str) -> None: """Pull latest changes from a git repository.""" subprocess.run( [ "git", "fetch", "--depth", "1", "origin", + "--", f"{branch}:refs/remotes/origin/{branch}", ], cwd=directory, check=True, capture_output=True, text=True, ) subprocess.run( - ["git", "checkout", "-B", branch, f"origin/{branch}"], + ["git", "checkout", "-B", branch, "--", f"origin/{branch}"], cwd=directory, capture_output=True, check=True, text=True, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/docs_buddy/adapters/__init__.py` around lines 139 - 169, The git operations in `_clone_repository` and `_update_repository` should be hardened so branch names starting with a dash are not parsed as options. Update the `subprocess.run` calls to use safe argument forms: pass the branch to `git clone` with `--branch=...` and ensure `git checkout` separates options from the branch/ref with `--` where applicable. Keep the behavior of `_clone_repository` and `_update_repository` unchanged otherwise, but make the branch handling unambiguous in these helper methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/docs_buddy/adapters/__init__.py`:
- Around line 139-169: Add text=True to the subprocess.run calls in
_clone_repository and _update_repository so captured stderr/stdout are decoded
as strings instead of bytes. This will ensure pull_repo and clone_repo surface
readable git error messages when they interpolate exc.stderr, and you should
keep the existing capture_output/check behavior unchanged while updating all git
subprocess invocations in this adapter.
In `@src/docs_buddy/common.py`:
- Around line 51-56: The sanitize_to_python_id function can crash on empty input
because it indexes sanitized[0] without checking for an empty string. Update
sanitize_to_python_id to handle empty identifiers (including the empty repo id
path from _derive_repo_id) before the digit check, and return a safe fallback
identifier when the input is blank so callers in __main__.py do not hit
IndexError.
In `@src/docs_buddy/entrypoints/cli/__main__.py`:
- Around line 175-178: The --query flow is reading optional config fields
directly, which can raise AttributeError when they are missing. In the CLI
entrypoint around the WhooshDocumentIndex setup, switch the accesses for
formatted_file_prefix and repo_content_description to getattr with a None
fallback, matching the optional handling already used elsewhere in the flow.
Keep the behavior aligned with WhooshDocumentIndex.__init__ and the existing
update-sources error handling so missing config entries do not crash the
command.
---
Nitpick comments:
In `@src/docs_buddy/adapters/__init__.py`:
- Around line 139-169: The git operations in `_clone_repository` and
`_update_repository` should be hardened so branch names starting with a dash are
not parsed as options. Update the `subprocess.run` calls to use safe argument
forms: pass the branch to `git clone` with `--branch=...` and ensure `git
checkout` separates options from the branch/ref with `--` where applicable. Keep
the behavior of `_clone_repository` and `_update_repository` unchanged
otherwise, but make the branch handling unambiguous in these helper methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8a5f3b9-8f3b-4851-82bb-25f1054fae80
📒 Files selected for processing (21)
.gitignoresrc/docs_buddy/_vendor/__init__.pysrc/docs_buddy/_vendor/dotconfig.pysrc/docs_buddy/adapters/__init__.pysrc/docs_buddy/adapters/agent.pysrc/docs_buddy/adapters/whoosh_index.pysrc/docs_buddy/common.pysrc/docs_buddy/entrypoints/bootstrap.pysrc/docs_buddy/entrypoints/cli/README.mdsrc/docs_buddy/entrypoints/cli/__main__.pysrc/docs_buddy/entrypoints/cli/data/docs_buddy_template.yamlsrc/docs_buddy/entrypoints/cli/requirements.txtsrc/docs_buddy/services/commands.pysrc/docs_buddy/services/events.pysrc/docs_buddy/services/handlers.pysrc/docs_buddy/services/use_cases.pytests/integration/test_adapters.pytests/unit/test_common.pytests/unit/test_services.pytests/unit/test_tools.pytodo.md
Comment on lines
+139
to
+169
| def _clone_repository(url: str, target_dir: str, branch: str) -> None: | ||
| """Clone a git repository with depth 1.""" | ||
| subprocess.run( | ||
| ["git", "clone", "--depth", "1", url, target_dir], | ||
| ["git", "clone", "--depth", "1", "--branch", branch, url, target_dir], | ||
| check=True, | ||
| capture_output=True, | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def _update_repository(directory: str) -> None: | ||
| def _update_repository(directory: str, branch: str) -> None: | ||
| """Pull latest changes from a git repository.""" | ||
| subprocess.run(["git", "pull"], cwd=directory, check=True, capture_output=True) | ||
| subprocess.run( | ||
| [ | ||
| "git", | ||
| "fetch", | ||
| "--depth", | ||
| "1", | ||
| "origin", | ||
| f"{branch}:refs/remotes/origin/{branch}", | ||
| ], | ||
| cwd=directory, | ||
| check=True, | ||
| capture_output=True, | ||
| ) | ||
|
|
||
| subprocess.run( | ||
| ["git", "checkout", "-B", branch, f"origin/{branch}"], | ||
| cwd=directory, | ||
| capture_output=True, | ||
| check=True, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add text=True to subprocess.run calls for clean error messages.
Without text=True, capture_output=True captures stderr as bytes. The error messages in pull_repo/clone_repo interpolate exc.stderr, so users will see b'fatal: ...' instead of fatal: ....
🔧 Proposed fix
`@staticmethod`
def _clone_repository(url: str, target_dir: str, branch: str) -> None:
"""Clone a git repository with depth 1."""
subprocess.run(
["git", "clone", "--depth", "1", "--branch", branch, url, target_dir],
check=True,
capture_output=True,
+ text=True,
)
`@staticmethod`
def _update_repository(directory: str, branch: str) -> None:
"""Pull latest changes from a git repository."""
subprocess.run(
[
"git",
"fetch",
"--depth",
"1",
"origin",
f"{branch}:refs/remotes/origin/{branch}",
],
cwd=directory,
check=True,
capture_output=True,
+ text=True,
)
subprocess.run(
["git", "checkout", "-B", branch, f"origin/{branch}"],
cwd=directory,
capture_output=True,
check=True,
+ text=True,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _clone_repository(url: str, target_dir: str, branch: str) -> None: | |
| """Clone a git repository with depth 1.""" | |
| subprocess.run( | |
| ["git", "clone", "--depth", "1", url, target_dir], | |
| ["git", "clone", "--depth", "1", "--branch", branch, url, target_dir], | |
| check=True, | |
| capture_output=True, | |
| ) | |
| @staticmethod | |
| def _update_repository(directory: str) -> None: | |
| def _update_repository(directory: str, branch: str) -> None: | |
| """Pull latest changes from a git repository.""" | |
| subprocess.run(["git", "pull"], cwd=directory, check=True, capture_output=True) | |
| subprocess.run( | |
| [ | |
| "git", | |
| "fetch", | |
| "--depth", | |
| "1", | |
| "origin", | |
| f"{branch}:refs/remotes/origin/{branch}", | |
| ], | |
| cwd=directory, | |
| check=True, | |
| capture_output=True, | |
| ) | |
| subprocess.run( | |
| ["git", "checkout", "-B", branch, f"origin/{branch}"], | |
| cwd=directory, | |
| capture_output=True, | |
| check=True, | |
| ) | |
| def _clone_repository(url: str, target_dir: str, branch: str) -> None: | |
| """Clone a git repository with depth 1.""" | |
| subprocess.run( | |
| ["git", "clone", "--depth", "1", "--branch", branch, url, target_dir], | |
| check=True, | |
| capture_output=True, | |
| text=True, | |
| ) | |
| `@staticmethod` | |
| def _update_repository(directory: str, branch: str) -> None: | |
| """Pull latest changes from a git repository.""" | |
| subprocess.run( | |
| [ | |
| "git", | |
| "fetch", | |
| "--depth", | |
| "1", | |
| "origin", | |
| f"{branch}:refs/remotes/origin/{branch}", | |
| ], | |
| cwd=directory, | |
| check=True, | |
| capture_output=True, | |
| text=True, | |
| ) | |
| subprocess.run( | |
| ["git", "checkout", "-B", branch, f"origin/{branch}"], | |
| cwd=directory, | |
| capture_output=True, | |
| check=True, | |
| text=True, | |
| ) |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 140-144: Command coming from incoming request
Context: subprocess.run(
["git", "clone", "--depth", "1", "--branch", branch, url, target_dir],
check=True,
capture_output=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 149-161: Command coming from incoming request
Context: subprocess.run(
[
"git",
"fetch",
"--depth",
"1",
"origin",
f"{branch}:refs/remotes/origin/{branch}",
],
cwd=directory,
check=True,
capture_output=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 163-168: Command coming from incoming request
Context: subprocess.run(
["git", "checkout", "-B", branch, f"origin/{branch}"],
cwd=directory,
capture_output=True,
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.20)
[error] 141-141: subprocess call: check for execution of untrusted input
(S603)
[error] 142-142: Starting a process with a partial executable path
(S607)
[error] 150-150: subprocess call: check for execution of untrusted input
(S603)
[error] 151-158: Starting a process with a partial executable path
(S607)
[error] 164-164: subprocess call: check for execution of untrusted input
(S603)
[error] 165-165: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/docs_buddy/adapters/__init__.py` around lines 139 - 169, Add text=True to
the subprocess.run calls in _clone_repository and _update_repository so captured
stderr/stdout are decoded as strings instead of bytes. This will ensure
pull_repo and clone_repo surface readable git error messages when they
interpolate exc.stderr, and you should keep the existing capture_output/check
behavior unchanged while updating all git subprocess invocations in this
adapter.
Comment on lines
+51
to
+56
| def sanitize_to_python_id(identifier: str) -> str: | ||
| """Converts string to valid Python identifier""" | ||
| sanitized = re.sub(r"[^a-zA-Z0-9_]", "_", identifier) | ||
| if sanitized[0].isdigit(): | ||
| sanitized = "name_" + sanitized | ||
| return sanitized |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard against empty string input in sanitize_to_python_id.
sanitized[0] raises IndexError when identifier is empty. If a config entry has an empty repo_url, _derive_repo_id returns "", which then crashes here when called from __main__.py:179.
🛡️ Proposed fix
def sanitize_to_python_id(identifier: str) -> str:
"""Converts string to valid Python identifier"""
sanitized = re.sub(r"[^a-zA-Z0-9_]", "_", identifier)
- if sanitized[0].isdigit():
+ if sanitized and sanitized[0].isdigit():
sanitized = "name_" + sanitized
return sanitized📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def sanitize_to_python_id(identifier: str) -> str: | |
| """Converts string to valid Python identifier""" | |
| sanitized = re.sub(r"[^a-zA-Z0-9_]", "_", identifier) | |
| if sanitized[0].isdigit(): | |
| sanitized = "name_" + sanitized | |
| return sanitized | |
| def sanitize_to_python_id(identifier: str) -> str: | |
| """Converts string to valid Python identifier""" | |
| sanitized = re.sub(r"[^a-zA-Z0-9_]", "_", identifier) | |
| if sanitized and sanitized[0].isdigit(): | |
| sanitized = "name_" + sanitized | |
| return sanitized |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/docs_buddy/common.py` around lines 51 - 56, The sanitize_to_python_id
function can crash on empty input because it indexes sanitized[0] without
checking for an empty string. Update sanitize_to_python_id to handle empty
identifiers (including the empty repo id path from _derive_repo_id) before the
digit check, and return a safe fallback identifier when the input is blank so
callers in __main__.py do not hit IndexError.
Comment on lines
+175
to
+178
| file_prefix = repo_config.formatted_file_prefix | ||
| document_index = adapters.WhooshDocumentIndex(index_path, file_prefix) | ||
|
|
||
| content_description = repo_config.repo_content_description |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use getattr fallback for optional config fields in --query flow.
formatted_file_prefix and repo_content_description are accessed without a fallback, but WhooshDocumentIndex.__init__ explicitly accepts None for file_prefix. This is inconsistent with the --update-sources flow (lines 142–145) which wraps file_extensions in a try/except. A config entry missing these fields will crash with an unhandled AttributeError.
🛡️ Proposed fix
- file_prefix = repo_config.formatted_file_prefix
+ file_prefix = getattr(repo_config, "formatted_file_prefix", None)
document_index = adapters.WhooshDocumentIndex(index_path, file_prefix)
- content_description = repo_config.repo_content_description
+ content_description = getattr(repo_config, "repo_content_description", "")
tool_id = common.sanitize_to_python_id(repo_id)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| file_prefix = repo_config.formatted_file_prefix | |
| document_index = adapters.WhooshDocumentIndex(index_path, file_prefix) | |
| content_description = repo_config.repo_content_description | |
| file_prefix = getattr(repo_config, "formatted_file_prefix", None) | |
| document_index = adapters.WhooshDocumentIndex(index_path, file_prefix) | |
| content_description = getattr(repo_config, "repo_content_description", "") | |
| tool_id = common.sanitize_to_python_id(repo_id) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/docs_buddy/entrypoints/cli/__main__.py` around lines 175 - 178, The
--query flow is reading optional config fields directly, which can raise
AttributeError when they are missing. In the CLI entrypoint around the
WhooshDocumentIndex setup, switch the accesses for formatted_file_prefix and
repo_content_description to getattr with a None fallback, matching the optional
handling already used elsewhere in the flow. Keep the behavior aligned with
WhooshDocumentIndex.__init__ and the existing update-sources error handling so
missing config entries do not crash the command.
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
1 participant
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.
This allows the user to specify configure repository sources using a config file and use the agent to do research on any relevant queries.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation