Skip to content

Conversation

@JamieMaclean
Copy link
Contributor

No description provided.

Copilot AI review requested due to automatic review settings December 19, 2025 22:15
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request introduces a comprehensive command-line interface (CLI) for the OpenFire fire engineering library, enabling users to create, manage, and run fire engineering projects from the terminal.

Key Changes:

  • Adds a new ofire CLI command with four subcommands: new, run, docs, and version
  • Implements project scaffolding that creates complete Streamlit-based fire engineering applications with virtual environments
  • Includes comprehensive test suite with cross-platform testing across Python 3.8-3.12 and Ubuntu/Windows/macOS

Reviewed changes

Copilot reviewed 13 out of 15 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
crates/python_api/ofire/cli.py Main CLI entry point defining argument parsing and command routing
crates/python_api/ofire/__main__.py Module entry point enabling python -m ofire execution
crates/python_api/ofire/__init__.py Package initialization importing Rust modules
crates/python_api/ofire/project.py Core project management logic including validation, scaffolding, and virtual environment setup
crates/python_api/ofire/templates/main.py Streamlit application template with smoke layer analysis example
crates/python_api/ofire/templates/agents_template.md AI agent guidance template for generated projects
crates/python_api/tests/test_cli.py Comprehensive CLI test suite covering all commands and cross-platform scenarios
crates/python_api/tests/conftest.py Pytest fixtures for CLI testing including temp directories and command runner
crates/python_api/tests/__init__.py Empty test package initialization
crates/python_api/pyproject.toml Adds CLI entry point, test dependencies, and pytest configuration
.github/workflows/test.yaml Extends CI to run CLI tests across multiple OS and Python versions
crates/python_api/docs/cli.rst Complete CLI documentation with examples and workflows
crates/python_api/docs/index.rst Updates documentation index to include CLI section
crates/python_api/README_TESTING.md Testing guide explaining how to run and develop CLI tests
Comments suppressed due to low confidence (1)

crates/python_api/ofire/project.py:3

  • Import of 'json' is not used.
import json

@@ -0,0 +1,439 @@
"""OpenFire project creation and management logic."""

import json
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The json module is imported but never used in this file. Remove the unused import to keep the code clean.

Suggested change
import json

Copilot uses AI. Check for mistakes.
Comment on lines +381 to +388
def scaffold_new_project(project_name: str, target_dir: str) -> None:
"""Scaffold a new OpenFire project with all required files and setup."""
try:
# Validate project name for security
validated_project_name = validate_project_name(project_name)

print(f"Creating OpenFire project: {validated_project_name}")
print(f"Target directory: {target_dir}")
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The target_dir parameter is not validated before use. A malicious user could potentially provide a path with special characters or path traversal sequences that, when combined with project_name via .resolve(), could lead to unexpected behavior. Consider adding validation to ensure target_dir is a valid, safe directory path before using it.

Copilot uses AI. Check for mistakes.
Comment on lines +343 to +361
def run_fire_app(target: str = None) -> None:
"""Run a fire engineering application."""
if target is None:
# Default to main.py
target = "main.py"
print("Running fire engineering app (main.py)")
elif target.startswith(('http://', 'https://')):
# URL provided
print(f"Running fire engineering app from URL: {target}")
elif os.path.exists(target):
# File path provided
print(f"Running fire engineering app: {target}")
else:
# Assume it's a file that might not exist yet
print(f"Running fire engineering app: {target}")

try:
# Build the command to run the web app
cmd = [sys.executable, '-m', 'streamlit', 'run', target]
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The target parameter is passed directly to subprocess without validation when it's a URL. This could allow arbitrary code execution if a malicious URL is provided. While streamlit itself may handle this, consider validating the URL format and potentially restricting to trusted domains, or at least warning the user before executing remote code.

Copilot uses AI. Check for mistakes.
[project.optional-dependencies]
test = [
"pytest>=7.0",
"pytest-subprocess>=1.5.0",
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pytest-subprocess>=1.5.0 dependency is listed in the test dependencies but is not imported or used anywhere in the test files. If this library is not needed for the current test implementation, consider removing it to reduce unnecessary dependencies. If it's intended for future use, add a comment explaining this.

Suggested change
"pytest-subprocess>=1.5.0",

Copilot uses AI. Check for mistakes.
def ofire_available():
"""Check if ofire CLI is available and properly installed."""
try:
result = subprocess.run(
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable result is not used.

Suggested change
result = subprocess.run(
subprocess.run(

Copilot uses AI. Check for mistakes.
import shutil
import subprocess
import sys
import os
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import of 'os' is not used.

Suggested change
import os

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,212 @@
import pytest
import subprocess
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import of 'subprocess' is not used.

Suggested change
import subprocess

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,212 @@
import pytest
import subprocess
import sys
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import of 'sys' is not used.

Suggested change
import sys

Copilot uses AI. Check for mistakes.
import subprocess
import sys
import os
from pathlib import Path
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import of 'Path' is not used.

Suggested change
from pathlib import Path

Copilot uses AI. Check for mistakes.
Comment on lines +104 to +105
except (subprocess.TimeoutExpired, subprocess.SubprocessError):
pass
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'except' clause does nothing but pass and there is no explanatory comment.

Suggested change
except (subprocess.TimeoutExpired, subprocess.SubprocessError):
pass
except (subprocess.TimeoutExpired, subprocess.SubprocessError) as exc:
# Failed to query pip for the latest ofire version; fall back to default.
print(
"Warning: Could not determine latest 'ofire' version from pip; "
"using default version 0.1.0.",
file=sys.stderr,
)
print(f" Reason: {exc}", file=sys.stderr)

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants