Harden LogSight-AI for production readiness#5
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe change broadens parser input handling, removes NumPy from anomaly calculations, adds validation and tests, modernizes packaging, replaces CI checks with quality/security/package/benchmark jobs, hardens container execution, and adds project, deployment, benchmarking, licensing, and security documentation. ChangesLogsight CLI and delivery controls
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9baabebeae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - run: ruff format --check . | ||
| - run: ruff check . |
There was a problem hiding this comment.
With this workflow change, both Ruff steps scan the entire repository, but the retained prototype files are not Ruff-clean. I ran the exact commands from the job: ruff format --check . wants to reformat agents/guards.py, agents/orchestrator.py, app.py, demo.app.py, and log_simulator.py, and ruff check . fails on existing import/whitespace/E701/F401 issues in those same demo/prototype areas. Since this job runs on every push/PR, CI is blocked until those files are formatted/fixed or the Ruff scope is limited to the supported package/tests.
Useful? React with 👍 / 👎.
| python-version: "3.11" | ||
| - run: pip install build | ||
| - run: python -m build | ||
| - run: docker build --tag logsight-ai:${{ github.sha }} . |
There was a problem hiding this comment.
The new container job now makes the existing Dockerfile part of required CI, but that builder stage only copies pyproject.toml, requirements.txt, and logsight/ before running python -m build; pyproject.toml declares readme = "README.md", so metadata generation inside the Docker build context is missing the README that exists at the repository root. In the package-and-container workflow I inspected, this docker build step will fail even though python -m build at the checkout root can see the file; copy README.md (and any other declared metadata files) into the builder stage before building the wheel.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/workflows/ci.yml:
- Line 20: Add persist-credentials: false to every actions/checkout@v4 step in
the workflow. Retain fetch-depth: 0 only on the security job’s checkout step,
removing it from other checkout steps while preserving their remaining
configuration.
- Around line 21-25: Update the CI quality job’s Python setup matrix to run both
Python 3.10 and 3.11, preserving the existing pip cache and development
dependency installation so the declared minimum version is validated.
In `@logsight/cli.py`:
- Line 60: Update the float option definitions in the CLI, including the one
using click.FloatRange(min=0), to use a shared validator that rejects NaN and
positive or negative infinity at Click parsing time. Ensure invalid non-finite
values become Click usage errors while preserving the existing range constraints
for finite inputs and applying the validator to unbounded threshold options as
well.
In `@README.md`:
- Around line 24-31: Update the README setup instructions around the
virtual-environment activation command by separating Unix/macOS and Windows
examples into distinct shell snippets. Include the existing Unix source command,
Windows Command Prompt activation, and PowerShell activation syntax, while
keeping the subsequent installation and Logsight usage commands clear and
unchanged.
🪄 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 Plus
Run ID: 2dd2eca5-69f2-486c-b4aa-d47f7a867641
📒 Files selected for processing (29)
.github/dependabot.yml.github/workflows/benchmarks.yml.github/workflows/ci-cd.yml.github/workflows/ci.yml.github/workflows/codeql.yml.github/workflows/data-validation.yml.github/workflows/release.yml.github/workflows/sast.yml.github/workflows/security.ymlDockerfileLICENSEREADME.mdSECURITY.mdagents/state.pybenchmarks/benchmark_report.mddocker-compose.ymldocs/AUDIT.mddocs/BENCHMARKING.mddocs/DEPLOYMENT.mdlogsight/analyzer.pylogsight/cli.pylogsight/parser.pypyproject.tomlrequirements-dev.txtrequirements.txttests/test_analyzer.pytests/test_benchmarks.pytests/test_cli.pytests/test_parser.py
💤 Files with no reviewable changes (6)
- .github/workflows/sast.yml
- .github/workflows/benchmarks.yml
- .github/workflows/security.yml
- .github/workflows/data-validation.yml
- .github/workflows/release.yml
- .github/workflows/ci-cd.yml
| steps: | ||
| - name: ⬇️ Checkout Repository | ||
| uses: actions/checkout@v4 | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workflow around all checkout steps.
git ls-files .github/workflows/ci.yml
echo "----"
nl -ba .github/workflows/ci.yml | sed -n '1,140p'
echo "----"
rg -n "actions/checkout@v4|persist-credentials|fetch-depth" .github/workflows/ci.ymlRepository: CoreyLeath-code/LogSight-AI
Length of output: 235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- file ----"
awk '{printf "%5d %s\n", NR, $0}' .github/workflows/ci.yml | sed -n '1,160p'
echo "---- matches ----"
rg -n "actions/checkout@v4|persist-credentials|fetch-depth" .github/workflows/ci.ymlRepository: CoreyLeath-code/LogSight-AI
Length of output: 3436
Disable persisted checkout credentials. These jobs run PR-controlled code, so leaving the default checkout token in git config lets that code read it. Set persist-credentials: false on every actions/checkout@v4 step, and keep fetch-depth: 0 only in the security job.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 20-20: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/ci.yml at line 20, Add persist-credentials: false to every
actions/checkout@v4 step in the workflow. Retain fetch-depth: 0 only on the
security job’s checkout step, removing it from other checkout steps while
preserving their remaining configuration.
Source: Linters/SAST tools
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.11" | ||
| cache: pip | ||
| - run: python -m pip install --upgrade pip && pip install -e ".[dev]" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'requires-python|python_version|python-version|FROM python:' \
pyproject.toml .github/workflows/ci.yml DockerfileRepository: CoreyLeath-code/LogSight-AI
Length of output: 589
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- .github/workflows/ci.yml ---\n'
cat -n .github/workflows/ci.yml
printf '\n--- pyproject.toml (relevant lines) ---\n'
cat -n pyproject.toml | sed -n '1,120p'
printf '\n--- Dockerfile ---\n'
cat -n DockerfileRepository: CoreyLeath-code/LogSight-AI
Length of output: 7308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- workflow checkout usage ---'
rg -n 'actions/checkout@|persist-credentials|python-version|python-version:' .github/workflows
echo
echo '--- any 3.10 references ---'
rg -n '3\.10|>=3\.10|python_version' .github pyproject.toml Dockerfile README.mdRepository: CoreyLeath-code/LogSight-AI
Length of output: 1040
Add Python 3.10 to CI pyproject.toml declares >=3.10, but this job only runs 3.11. Run the quality job on 3.10 too so minimum-version regressions are caught.
🤖 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 @.github/workflows/ci.yml around lines 21 - 25, Update the CI quality job’s
Python setup matrix to run both Python 3.10 and 3.11, preserving the existing
pip cache and development dependency installation so the declared minimum
version is validated.
| "--threshold", | ||
| "-t", | ||
| default=2.5, | ||
| type=click.FloatRange(min=0), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== logsight/cli.py (relevant sections) ==\n'
cat -n logsight/cli.py | sed -n '1,220p'
printf '\n== search for float options and downstream use ==\n'
rg -n "FloatRange|detect_anomalies|error_rate_spike|nan|inf|finite|math\.isfinite" logsight -S
printf '\n== locate tests for CLI validation ==\n'
rg -n "click\.CliRunner|CliRunner|invalid value|FloatRange|nan|inf|detect_anomalies|error_rate_spike" tests logsight -SRepository: CoreyLeath-code/LogSight-AI
Length of output: 10487
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== logsight/analyzer.py (relevant sections) ==\n'
cat -n logsight/analyzer.py | sed -n '1,220p'
printf '\n== tests/test_cli.py ==\n'
cat -n tests/test_cli.py | sed -n '1,220p'
printf '\n== click FloatRange behavior probe ==\n'
python3 - <<'PY'
import click
from click.testing import CliRunner
`@click.command`()
`@click.option`("--x", type=click.FloatRange(min=0), default=2.5)
def cmd(x):
click.echo(repr(x))
runner = CliRunner()
for arg in ["inf", "-inf", "nan", "1.5", "0", "-1"]:
result = runner.invoke(cmd, ["--x", arg])
print(arg, "=>", "OK" if result.exit_code == 0 else f"ERR {result.exit_code}", repr(result.output.strip()))
PYRepository: CoreyLeath-code/LogSight-AI
Length of output: 9187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import click
from click.testing import CliRunner
`@click.command`()
`@click.option`("--x", type=click.FloatRange(min=0, max=1))
def cmd(x):
click.echo(repr(x))
runner = CliRunner()
for arg in ["inf", "-inf", "nan", "1.5", "1", "0"]:
result = runner.invoke(cmd, ["--x", arg])
status = "OK" if result.exit_code == 0 else f"ERR {result.exit_code}"
print(f"{arg:>4} => {status} | {result.output.strip()!r}")
PYRepository: CoreyLeath-code/LogSight-AI
Length of output: 628
Reject non-finite float inputs at the Click boundary. FloatRange still lets nan through, and the unbounded threshold options also accept inf; both end up as raw ValueErrors in detect_anomalies/error_rate_spike instead of Click usage errors. Add a shared finite-value validator for the float options here.
🤖 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 `@logsight/cli.py` at line 60, Update the float option definitions in the CLI,
including the one using click.FloatRange(min=0), to use a shared validator that
rejects NaN and positive or negative infinity at Click parsing time. Ensure
invalid non-finite values become Click usage errors while preserving the
existing range constraints for finite inputs and applying the validator to
unbounded threshold options as well.
| ```bash | ||
| python -m venv .venv | ||
| source .venv/bin/activate # Windows: .venv\Scripts\activate | ||
| pip install -e . | ||
| logsight health | ||
| logsight analyze application.log | ||
| cat application.log | logsight stdin | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Separate Unix and Windows activation commands.
The code block uses source .venv/bin/activate while only commenting on the Windows path; Windows users cannot paste this command as written. Provide separate shell snippets, including PowerShell activation syntax.
🤖 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 `@README.md` around lines 24 - 31, Update the README setup instructions around
the virtual-environment activation command by separating Unix/macOS and Windows
examples into distinct shell snippets. Include the existing Unix source command,
Windows Command Prompt activation, and PowerShell activation syntax, while
keeping the subsequent installation and Logsight usage commands clear and
unchanged.
2 participants
Summary
Production-hardens the supported LogSight-AI CLI and replaces false-green automation with enforceable quality, security, packaging, benchmark, and container gates.
What changed
Why
The previous repository could report success after failed tests, security scans, and benchmarks; advertised unsupported deployment services; and accepted invalid analysis parameters. This change makes the supported local CLI contract reproducible and reviewable.
Validation
Docker image execution remains delegated to CI because the local Docker daemon was unavailable.
Summary by CodeRabbit
stdin).