Skip to content

Commit 95749bf

Browse files
committed
fix: address CodeRabbitAI review comments
- Use argparse for CLI argument parsing in server.py - Use cross-platform webbrowser command in CONTRIBUTING.md - Fix agent count and hyphenation in tasks file - Fix test ordering assertions to follow SUPPORTED_AGENTS order - Add explicit UTF-8 encoding to conftest.py - Use iter_detection_dirs() in detection.py - Clarify TOML dependency documentation - Remove Qwen Code reference from docs - Simplify test_writer.py fixture - Add command prefix assertion in test_validation.py - Show actual agent keys in error messages - Preserve intentional blank lines in normalization - Add argument deduplication in generators.py
1 parent 620f797 commit 95749bf

File tree

11 files changed

+79
-96
lines changed

11 files changed

+79
-96
lines changed

CONTRIBUTING.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,8 @@ See `docs/operations.md` for more details on transports and configuration.
6262
# Run tests with coverage report
6363
uv run pytest
6464

65-
# View HTML coverage report (opens in browser)
66-
open htmlcov/index.html
65+
# View HTML coverage report (opens in default browser)
66+
uv run python -m webbrowser htmlcov/index.html
6767
```
6868

6969
The test suite generates both terminal and HTML coverage reports showing which code paths are tested.

docs/slash-command-generator.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ This means:
5555
- ✅ Guaranteed compatibility with your Python installation
5656
- ✅ Faster import times
5757

58+
**Note**: While `tomllib` handles parsing TOML files (reading), the project uses `tomli-w` for writing TOML files (generating command files for Gemini CLI). Both are lightweight dependencies and `tomli-w` is required for generating TOML command files.
59+
5860
### Running Commands
5961

6062
After installation, use `uv run` to execute the command:
@@ -223,7 +225,7 @@ $ARGUMENTS
223225

224226
### TOML Format
225227

226-
TOML-based agents (Gemini CLI, Qwen Code) use TOML syntax:
228+
TOML-based agents (Gemini CLI) use TOML syntax:
227229

228230
```toml
229231
[command]

server.py

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
The 'mcp' instance is automatically discovered by the FastMCP CLI.
55
"""
66

7-
import sys
7+
import argparse
88

99
from mcp_server import create_app
1010

@@ -22,25 +22,24 @@ def main() -> None:
2222
It runs the MCP server using stdio transport by default, or http transport
2323
if --transport http is passed as an argument.
2424
"""
25-
# Parse command line arguments
26-
transport = "stdio"
27-
port = 8000
28-
args = sys.argv[1:]
29-
30-
# Simple argument parsing for transport and port
31-
if "--transport" in args:
32-
idx = args.index("--transport")
33-
if idx + 1 < len(args):
34-
transport = args[idx + 1]
35-
36-
if "--port" in args:
37-
idx = args.index("--port")
38-
if idx + 1 < len(args):
39-
port = int(args[idx + 1])
25+
parser = argparse.ArgumentParser(description="Run the MCP server")
26+
parser.add_argument(
27+
"--transport",
28+
choices=["stdio", "http"],
29+
default="stdio",
30+
help="Transport type (default: stdio)",
31+
)
32+
parser.add_argument(
33+
"--port",
34+
type=int,
35+
default=8000,
36+
help="HTTP server port (default: 8000)",
37+
)
38+
args = parser.parse_args()
4039

4140
# Run the server with the specified transport
42-
if transport == "http":
43-
mcp.run(transport="http", port=port)
41+
if args.transport == "http":
42+
mcp.run(transport="http", port=args.port)
4443
else:
4544
mcp.run()
4645

slash_commands/cli.py

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -218,9 +218,7 @@ def generate( # noqa: PLR0913 PLR0912 PLR0915
218218
print("\nTo fix this:", file=sys.stderr)
219219
print(" - Use --list-agents to see all supported agents", file=sys.stderr)
220220
print(" - Ensure agent keys are spelled correctly", file=sys.stderr)
221-
print(
222-
" - Valid agent keys include: claude-code, cursor, gemini-cli, etc.", file=sys.stderr
223-
)
221+
print(f" - Valid agent keys: {', '.join(list_agent_keys())}", file=sys.stderr)
224222
sys.exit(2) # Validation error (invalid agent key)
225223
except PermissionError as e:
226224
print(f"Error: Permission denied: {e}", file=sys.stderr)

slash_commands/detection.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ def iter_detection_directories(agent: AgentConfig, base_path: Path | str) -> Ite
3535
"""Yield absolute paths for the agent's detection directories."""
3636

3737
base = Path(base_path)
38-
for directory in agent.detection_dirs:
38+
for directory in agent.iter_detection_dirs():
3939
yield base / Path(directory)
4040

4141

slash_commands/generators.py

Lines changed: 12 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
from __future__ import annotations
44

5-
from datetime import datetime
5+
from datetime import UTC, datetime
66
from typing import Any, Protocol
77

88
import tomli_w
@@ -47,8 +47,12 @@ def _apply_agent_overrides(
4747
if "arguments" in overrides:
4848
# Merge base arguments with override arguments
4949
override_args = _normalize_override_arguments(overrides["arguments"])
50-
# Combine base args with override args (override args are appended)
51-
arguments = list(arguments) + override_args
50+
# Deduplicate by name with override precedence
51+
existing_names = {arg.name for arg in arguments}
52+
# Only add override args that don't already exist
53+
arguments = list(arguments) + [
54+
arg for arg in override_args if arg.name not in existing_names
55+
]
5256
if "enabled" in overrides:
5357
enabled = overrides["enabled"]
5458

@@ -78,7 +82,7 @@ def _normalize_output(content: str) -> str:
7882
- Ensures consistent line endings (LF)
7983
- Removes trailing whitespace from lines
8084
- Ensures UTF-8 encoding
81-
- Normalizes multiple blank lines to single blank line
85+
- Preserves intentional blank lines
8286
8387
Args:
8488
content: The generated content to normalize
@@ -89,23 +93,8 @@ def _normalize_output(content: str) -> str:
8993
# Normalize line endings to LF
9094
content = content.replace("\r\n", "\n").replace("\r", "\n")
9195

92-
# Remove trailing whitespace from each line
93-
lines = []
94-
for line in content.splitlines():
95-
lines.append(line.rstrip())
96-
97-
# Normalize multiple consecutive blank lines to single blank line
98-
normalized_lines = []
99-
prev_blank = False
100-
for line in lines:
101-
is_blank = not line.strip()
102-
if is_blank and prev_blank:
103-
continue # Skip consecutive blank lines
104-
normalized_lines.append(line)
105-
prev_blank = is_blank
106-
107-
# Join lines and ensure trailing newline
108-
result = "\n".join(normalized_lines)
96+
# Remove trailing whitespace from each line, preserve intentional blank lines
97+
result = "\n".join(line.rstrip() for line in content.splitlines())
10998
if result and not result.endswith("\n"):
11099
result += "\n"
111100

@@ -226,7 +215,7 @@ def _build_meta(self, prompt: MarkdownPrompt, agent: AgentConfig) -> dict:
226215
"source_prompt": prompt.name,
227216
"source_path": str(prompt.path),
228217
"version": __version__,
229-
"updated_at": datetime.now().isoformat(),
218+
"updated_at": datetime.now(UTC).isoformat(),
230219
})
231220
return meta
232221

@@ -265,7 +254,7 @@ def generate(self, prompt: MarkdownPrompt, agent: AgentConfig) -> str:
265254
# These are ignored by Gemini CLI but preserved for bookkeeping
266255
toml_data["meta"] = {
267256
"version": __version__,
268-
"updated_at": datetime.now().isoformat(),
257+
"updated_at": datetime.now(UTC).isoformat(),
269258
"source_prompt": prompt.name,
270259
"agent": agent.key,
271260
}

tasks/tasks-0003-spec-slash-command-generator.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,11 @@
2525
## Tasks
2626

2727
- [x] 1.0 Establish slash command configuration and agent detection foundations
28-
- Demo Criteria: "Config data models enumerate all 14 agents with accurate directories/formats and detection flags configured tools under pytest validation."
28+
- Demo Criteria: "Config data models enumerate all 6 agents with accurate directories/formats and detection flags configured tools under pytest validation."
2929
- Proof Artifact(s): "CLI: `pytest tests/test_config.py tests/test_detection.py -v`; Log: detection fixture output listing detected agents."
3030
- [x] 1.1 Author failing tests in `tests/test_config.py` that assert required fields and format values for every agent entry.
3131
- [x] 1.2 Implement `CommandFormat` enum, `AgentConfig` dataclass, and helper accessors in `slash_commands/config.py` to satisfy the tests.
32-
- [x] 1.3 Populate `SUPPORTED_AGENTS` with all 14 tools, including directory paths, file extensions, and format metadata.
32+
- [x] 1.3 Populate `SUPPORTED_AGENTS` with all 6 tools, including directory paths, file extensions, and format metadata.
3333
- [x] 1.4 Draft failing detection tests in `tests/test_detection.py` covering positive, negative, and mixed directory scenarios using `tmp_path` fixtures.
3434
- [x] 1.5 Implement `detect_agents` (and supporting utilities) in `slash_commands/detection.py` so detection tests pass with deterministic ordering.
3535

@@ -42,7 +42,7 @@
4242
- [x] 2.4 Implement `CommandGenerator` base class plus Markdown and TOML subclasses in `slash_commands/generators.py`, including helper factory selection logic.
4343
- [x] 2.5 Refine generators to normalize whitespace and encoding, updating tests to use snapshot-style comparisons for regression safety.
4444

45-
- [x] 3.0 Build slash command writer orchestrating multi-agent generation and dry runs
45+
- [x] 3.0 Build slash command writer orchestrating multiagent generation and dry runs
4646
- Demo Criteria: "Writer loads prompts, generates commands for single and multi-agent selections, ensures directories exist, and reports dry-run results without writes."
4747
- Proof Artifact(s): "CLI: `pytest tests/test_writer.py -v`; Log: dry-run test output showing file paths and counts."
4848
- [x] 3.1 Introduce failing writer tests that mock prompt loading and assert correct call sequences for single and multi-agent runs.

tests/conftest.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,8 @@ def sample_prompt(tmp_path) -> MarkdownPrompt:
136136
137137
Use the provided instructions to perform the desired action.
138138
"""
139-
)
139+
),
140+
encoding="utf-8",
140141
)
141142

142143
return load_markdown_prompt(prompt_path)
@@ -178,7 +179,8 @@ def prompt_with_placeholder_body(tmp_path) -> MarkdownPrompt:
178179
179180
and ensure `{{args}}` are handled correctly.
180181
"""
181-
)
182+
),
183+
encoding="utf-8",
182184
)
183185

184186
return load_markdown_prompt(prompt_path)

tests/test_detection.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@ def test_detect_agents_identifies_configured_directories(
3434
detected = detect_agents(tmp_path)
3535
detected_keys = [agent.key for agent in detected]
3636

37-
assert detected_keys == sorted(agent_keys)
37+
expected_order = [a.key for a in SUPPORTED_AGENTS if a.key in agent_keys]
38+
assert detected_keys == expected_order
3839
for key in detected_keys:
3940
directories = {tmp_path / path for path in supported_agents_by_key[key].detection_dirs}
4041
assert all(directory.exists() for directory in directories)

tests/test_validation.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ def test_yaml_frontmatter_parsing(sample_prompt):
5252
# Verify frontmatter is a dict and contains required fields
5353
assert isinstance(frontmatter, dict)
5454
assert "name" in frontmatter
55+
assert frontmatter["name"].startswith("sdd-"), "Expected command name to include prefix"
5556
assert "description" in frontmatter
5657
assert "tags" in frontmatter
5758
assert "enabled" in frontmatter
@@ -180,6 +181,13 @@ def test_generated_content_is_valid_before_writing(sample_prompt):
180181
assert isinstance(toml_data, dict)
181182
assert "prompt" in toml_data
182183

184+
# Verify TOML meta includes updated_at
185+
assert "meta" in toml_data
186+
assert "updated_at" in toml_data["meta"]
187+
updated_at = toml_data["meta"]["updated_at"]
188+
assert isinstance(updated_at, str), "Expected updated_at to be a string"
189+
# Note: datetime formatting with timezone ensures ISO-8601 compliance
190+
183191
# Both should be valid before any file writing occurs
184192
assert len(markdown_content) > 0
185193
assert len(toml_content) > 0

0 commit comments

Comments
 (0)