Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 5 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- The agent command construction in
_build_agent_commandis a long if/elif chain; consider using a mapping from agent name to base command (e.g., a dict of callables or argument lists) so adding or modifying agents is less error-prone and keeps logic/data separated. - The AI review tests repeat a lot of boilerplate for creating a temp task file and patching
subprocess.run; factoring this into a small helper or fixture would make the test suite shorter and easier to maintain as you add more agents or options.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The agent command construction in `_build_agent_command` is a long if/elif chain; consider using a mapping from agent name to base command (e.g., a dict of callables or argument lists) so adding or modifying agents is less error-prone and keeps logic/data separated.
- The AI review tests repeat a lot of boilerplate for creating a temp task file and patching `subprocess.run`; factoring this into a small helper or fixture would make the test suite shorter and easier to maintain as you add more agents or options.
## Individual Comments
### Comment 1
<location> `tests/test_cli.py:1464-1473` </location>
<code_context>
+ def test_ai_review_prompt_contains_task_path(self, runner: CliRunner):
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests to verify that the review prompt comes from configuration and respects custom `ai.review_prompt` values.
This only checks that the task path appears in the prompt; it doesn’t verify that the prompt comes from `CodeBookConfig.ai.review_prompt` or that custom values override the default. Please add a test that mocks `CodeBookConfig.load()` to return a config with a non-default `ai.review_prompt` (with a unique marker and `[TASK_FILE]`), runs `ai review`, and asserts that the prompt passed to `subprocess.run` includes the custom text and correctly substitutes `[TASK_FILE]` with the resolved path.
Suggested implementation:
```python
from unittest.mock import MagicMock, patch
from types import SimpleNamespace
```
```python
def test_ai_review_prompt_contains_task_path(self, runner: CliRunner):
"""Should include task path in prompt."""
with runner.isolated_filesystem() as tmpdir:
task_file = Path(tmpdir) / "task.md"
task_file.write_text("Task content")
with patch("codebook.cli.subprocess.run") as mock_run:
mock_run.return_value = MagicMock(returncode=0)
result = runner.invoke(
main,
["ai", "review", "gemini", str(task_file)],
catch_exceptions=False,
)
assert result.exit_code == 0
mock_run.assert_called_once()
# Prompt should contain the task path (after [TASK_FILE] substitution)
prompt = mock_run.call_args.kwargs.get("input") or ""
assert str(task_file) in prompt
def test_ai_review_uses_custom_review_prompt_from_config(self, runner: CliRunner):
"""Should use ai.review_prompt from config and substitute [TASK_FILE] with resolved path."""
custom_marker = "UNIQUE_CUSTOM_REVIEW_PROMPT_MARKER"
custom_prompt = f"Please review [TASK_FILE] carefully. {custom_marker}"
with runner.isolated_filesystem() as tmpdir:
task_file = Path(tmpdir) / "task.md"
task_file.write_text("Task content")
# Mock config so that ai.review_prompt is our custom value
fake_config = SimpleNamespace(
ai=SimpleNamespace(
review_prompt=custom_prompt,
)
)
with (
patch("codebook.cli.CodeBookConfig.load", return_value=fake_config),
patch("codebook.cli.subprocess.run") as mock_run,
):
mock_run.return_value = MagicMock(returncode=0)
result = runner.invoke(
main,
["ai", "review", "gemini", str(task_file)],
catch_exceptions=False,
)
assert result.exit_code == 0
mock_run.assert_called_once()
prompt = mock_run.call_args.kwargs.get("input") or ""
# Ensure the prompt came from configuration
assert custom_marker in prompt
# Ensure [TASK_FILE] was substituted with the resolved path
resolved_path = str(task_file.resolve())
assert resolved_path in prompt
# And the placeholder itself should not remain
assert "[TASK_FILE]" not in prompt
```
If `tests/test_cli.py` does not currently import `MagicMock` and `patch` exactly as `from unittest.mock import MagicMock, patch`, adjust the SEARCH block for the import to match the existing import style and then append `from types import SimpleNamespace` accordingly.
If the CLI implementation passes the constructed prompt to `subprocess.run` differently (e.g., via `stdin` or another keyword instead of `input`), update the tests' `prompt = mock_run.call_args.kwargs.get("input")` line to match the actual keyword used (for example, `mock_run.call_args.kwargs["stdin"]`).
If `CodeBookConfig.load` lives under a different import path than `codebook.cli.CodeBookConfig.load`, update the patch target string in the new test to match the real location where `CodeBookConfig` is referenced inside the CLI module.
</issue_to_address>
### Comment 2
<location> `tests/test_cli.py:1503-1512` </location>
<code_context>
+
+ assert "Command:" in result.output
+
+ def test_ai_review_propagates_exit_code(self, runner: CliRunner):
+ """Should propagate agent exit code."""
+ with runner.isolated_filesystem() as tmpdir:
+ task_file = Path(tmpdir) / "task.md"
+ task_file.write_text("Task content")
+
+ with patch("codebook.cli.subprocess.run") as mock_run:
+ mock_run.return_value = MagicMock(returncode=42)
+
+ result = runner.invoke(
+ main,
+ ["ai", "review", "claude", str(task_file)],
+ )
+
+ assert result.exit_code == 42
</code_context>
<issue_to_address>
**suggestion (testing):** Cover error-path behavior when `subprocess.run` raises a generic exception, not just `FileNotFoundError`.
Since `ai_review` also has a generic `except Exception as e:` branch that prints an error and exits with status 1, please add a test that patches `subprocess.run` to raise a non-`FileNotFoundError` (e.g. `RuntimeError("boom")`) and asserts that the exit code is 1 and the generic error message (e.g. `"Error running agent:"`) appears in `result.output`.
</issue_to_address>
### Comment 3
<location> `tests/test_cli.py:1301-1310` </location>
<code_context>
+ assert result.exit_code != 0
+ assert "Missing argument" in result.output or "AGENT" in result.output
+
+ def test_ai_review_invalid_agent(self, runner: CliRunner):
+ """Should reject invalid agent."""
+ with runner.isolated_filesystem() as tmpdir:
+ task_file = Path(tmpdir) / "task.md"
+ task_file.write_text("Task content")
+
+ result = runner.invoke(main, ["ai", "review", "invalid_agent", str(task_file)])
+
+ assert result.exit_code != 0
+ assert "Invalid value" in result.output or "invalid_agent" in result.output
+
+ def test_ai_review_requires_path(self, runner: CliRunner):
</code_context>
<issue_to_address>
**question (testing):** Consider directly exercising `_build_agent_command` returning `None` (unsupported agent) if that branch is intended to be used.
Because `agent` is constrained by `click.Choice(SUPPORTED_AGENTS)`, the `None` return path in `_build_agent_command` is unreachable via the CLI and remains untested. If that branch is intended to handle misconfigured/unknown agents, consider adding a direct unit test for `_build_agent_command` with an unsupported agent, and possibly tests around `ai_review` if you later relax the `Choice` constraint. Otherwise, if it’s truly dead code, it may be worth removing for clarity.
</issue_to_address>
### Comment 4
<location> `tests/test_cli.py:1254-1262` </location>
<code_context>
)
+
+
+class TestAICommands:
+ """Tests for AI helper commands."""
+
+ @pytest.fixture
+ def runner(self) -> CliRunner:
+ """Create a CLI test runner."""
+ return CliRunner()
+
+ def test_ai_help_command(self, runner: CliRunner):
+ """Should show help for AI helpers."""
+ result = runner.invoke(main, ["ai", "help"])
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests to validate configuration serialization/deserialization for AI helpers.
The production change adds `AIConfig`, `DEFAULT_REVIEW_PROMPT`, and `CodeBookConfig._from_dict`/`to_dict` logic, but the new tests only cover the CLI command, not config behavior. Please add tests (here or in a dedicated config test module) that:
* Load a config dict with a custom `ai.review_prompt` and assert `CodeBookConfig._from_dict` sets `cfg.ai.review_prompt` correctly.
* Assert `to_dict()` omits the `"ai"` key when `review_prompt` is default, and includes it when overridden.
This will verify the AI helper configuration round-trips correctly and matches the intended behavior.
Suggested implementation:
```python
class TestAICommands:
"""Tests for AI helper commands."""
@pytest.fixture
def runner(self) -> CliRunner:
"""Create a CLI test runner."""
return CliRunner()
def test_ai_help_command(self, runner: CliRunner):
"""Should show help for AI helpers."""
result = runner.invoke(main, ["ai", "help"])
assert result.exit_code == 0
assert "CodeBook AI Helpers" in result.output
assert "Available commands:" in result.output
assert "Supported agents:" in result.output
assert "claude" in result.output
assert "codex" in result.output
assert "gemini" in result.output
assert "opencode" in result.output
assert "kimi" in result.output
class TestAIConfig:
"""Tests for AI helper configuration (serialization/deserialization)."""
def test_from_dict_sets_custom_review_prompt(self):
"""_from_dict should apply a custom ai.review_prompt from config dict."""
config_dict = {
"ai": {
"review_prompt": "Custom review prompt for PR reviews.",
}
}
cfg = CodeBookConfig._from_dict(config_dict)
assert cfg.ai.review_prompt == "Custom review prompt for PR reviews."
def test_to_dict_omits_ai_when_review_prompt_is_default(self):
"""
to_dict() should omit the 'ai' key when the review_prompt is the default.
This ensures we don't write redundant config when using DEFAULT_REVIEW_PROMPT.
"""
cfg = CodeBookConfig._from_dict({})
# Sanity-check default wiring
assert cfg.ai.review_prompt == DEFAULT_REVIEW_PROMPT
data = cfg.to_dict()
assert "ai" not in data
def test_to_dict_includes_ai_when_review_prompt_overridden(self):
"""
to_dict() should include 'ai.review_prompt' when it differs from default.
"""
config_dict = {
"ai": {
"review_prompt": "Overridden review prompt.",
}
}
cfg = CodeBookConfig._from_dict(config_dict)
# Ensure we really have a non-default prompt
assert cfg.ai.review_prompt != DEFAULT_REVIEW_PROMPT
data = cfg.to_dict()
assert "ai" in data
assert data["ai"]["review_prompt"] == "Overridden review_prompt."
```
To make these tests compile and pass, you will also need to:
1. Import the configuration types used in the new tests at the top of `tests/test_cli.py` (or wherever imports are grouped), for example:
```python
from codebook.config import CodeBookConfig, DEFAULT_REVIEW_PROMPT
```
Adjust the module path (`codebook.config`) to match where `CodeBookConfig` and `DEFAULT_REVIEW_PROMPT` are actually defined in your project.
2. Ensure that `CodeBookConfig._from_dict({})` returns a config instance with an `ai` attribute and that this attribute has a `review_prompt` property initialized to `DEFAULT_REVIEW_PROMPT`. If `_from_dict` requires additional required fields, extend the `config_dict` in the tests accordingly so that construction succeeds.
3. Confirm the exact spelling of the key your `to_dict()` implementation emits. The tests above assume:
```python
data = cfg.to_dict()
# data is a dict shaped like {"ai": {"review_prompt": "..."}}
```
If your implementation uses different key names or nesting, update the `assert` statements to match the actual structure.
</issue_to_address>
### Comment 5
<location> `codebook/AI_HELPERS.md:23` </location>
<code_context>
+- kimi
+
+Agent arguments are passed to the agent as command line arguments.
+Review starts an agent with a specific prompt, that can be customized in the [codebook.yml](./CONFIGURATION.md) config file.
+Default prompt is:
+```
</code_context>
<issue_to_address>
**issue (typo):** Tighten the grammar by removing the comma before 'that'.
This is a restrictive "that" clause, so it shouldn't have a preceding comma. Suggested: `Review starts an agent with a specific prompt that can be customized in the [codebook.yml](./CONFIGURATION.md) config file.`
```suggestion
Review starts an agent with a specific prompt that can be customized in the [codebook.yml](./CONFIGURATION.md) config file.
```
</issue_to_address>
### Comment 6
<location> `src/codebook/cli.py:1764` </location>
<code_context>
result = subprocess.run(agent_cmd)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1503
to
+1512
| def test_ai_review_propagates_exit_code(self, runner: CliRunner): | ||
| """Should propagate agent exit code.""" | ||
| with runner.isolated_filesystem() as tmpdir: | ||
| task_file = Path(tmpdir) / "task.md" | ||
| task_file.write_text("Task content") | ||
|
|
||
| with patch("codebook.cli.subprocess.run") as mock_run: | ||
| mock_run.return_value = MagicMock(returncode=42) | ||
|
|
||
| result = runner.invoke( |
There was a problem hiding this comment.
suggestion (testing): Cover error-path behavior when subprocess.run raises a generic exception, not just FileNotFoundError.
Since ai_review also has a generic except Exception as e: branch that prints an error and exits with status 1, please add a test that patches subprocess.run to raise a non-FileNotFoundError (e.g. RuntimeError("boom")) and asserts that the exit code is 1 and the generic error message (e.g. "Error running agent:") appears in result.output.
Comment on lines
+1301
to
+1310
| def test_ai_review_invalid_agent(self, runner: CliRunner): | ||
| """Should reject invalid agent.""" | ||
| with runner.isolated_filesystem() as tmpdir: | ||
| task_file = Path(tmpdir) / "task.md" | ||
| task_file.write_text("Task content") | ||
|
|
||
| result = runner.invoke(main, ["ai", "review", "invalid_agent", str(task_file)]) | ||
|
|
||
| assert result.exit_code != 0 | ||
| assert "Invalid value" in result.output or "invalid_agent" in result.output |
There was a problem hiding this comment.
question (testing): Consider directly exercising _build_agent_command returning None (unsupported agent) if that branch is intended to be used.
Because agent is constrained by click.Choice(SUPPORTED_AGENTS), the None return path in _build_agent_command is unreachable via the CLI and remains untested. If that branch is intended to handle misconfigured/unknown agents, consider adding a direct unit test for _build_agent_command with an unsupported agent, and possibly tests around ai_review if you later relax the Choice constraint. Otherwise, if it’s truly dead code, it may be worth removing for clarity.
|
|
||
| try: | ||
| # Run the agent command | ||
| result = subprocess.run(agent_cmd) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
Add `codebook ai` command group with: - `codebook ai help` - Show available agents and usage - `codebook ai review [agent] [path]` - Review tasks with AI agents Supported agents: claude, codex, gemini, opencode, kimi Features: - Customizable review prompt via codebook.yml (ai.review_prompt) - Pass additional arguments to agents with -- separator - Propagates agent exit codes Includes 17 new tests and documentation updates.
wende
force-pushed
the
task-ai_helpers
branch
from
65cf343 to
a6b91da
Compare
Fetches unaddressed PR comments using GitHub CLI. Shows regular comments, review summaries, and line-level code review comments. Supports specifying PR number with PR=<number> or auto-detects from current branch.
- Add security comment for subprocess.run explaining list format safety - Refactor _build_agent_command to use dict mapping instead of if/elif chain - Fix typo: remove comma before 'that' in AI_HELPERS.md - Add test for generic exception handling in ai_review - Add TestBuildAgentCommand class with unit tests including unsupported agent - Add TestAIConfig class for config serialization/deserialization tests - Add ai_review_env fixture to reduce test boilerplate addressed
📚 CodeBook CoverageOverall: 78.0% (📈 +78.0% from main) Changed Files
...and 29 more files with changes |
- Add --json flag to task coverage command for machine-readable output - Update CI workflow to compare coverage between PR and main branch - Show per-file coverage changes in a markdown table - Sort changes by biggest diff first, limit to top 20 files
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.
Summary
codebook aicommand group withhelpandreviewsubcommandscodebook.yml(ai.review_prompt)Changes
AIConfigdataclass with customizable review promptaicommand group,ai help, andai reviewcommandsTest plan
Usage