Add analyze_text() and analyze_lines() API with stdin support#39
Merged
Conversation
|
Warning Review limit reached
Your plan currently allows 2 reviews/hour. Refill in 22 minutes and 17 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds support for analyzing logs from standard input by introducing text-based analysis methods to ChangesStdin Log Analysis Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (3)
src/cordon/pipeline.py (1)
121-130: ⚡ Quick winHarden
analyze_lines()input to prevent silent line collisions.At Line 121, this public API accepts arbitrary tuples, but downstream formatting uses
dict(lines), so duplicate line numbers overwrite earlier content silently. Add a lightweight validation for strictly increasing, unique line numbers before forwarding.Proposed change
def analyze_lines(self, lines: Sequence[tuple[int, str]]) -> AnalysisResult: @@ - return self._analyze_lines(list(lines)) + lines_list = list(lines) + line_numbers = [line_number for line_number, _ in lines_list] + if line_numbers != sorted(line_numbers): + raise ValueError("lines must be sorted by ascending line number") + if len(set(line_numbers)) != len(line_numbers): + raise ValueError("lines must not contain duplicate line numbers") + return self._analyze_lines(lines_list)🤖 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/cordon/pipeline.py` around lines 121 - 130, The public method analyze_lines currently accepts arbitrary (line_number, line_content) tuples and forwards them to _analyze_lines after converting to list, which can silently allow duplicate or out-of-order line numbers (downstream uses dict(lines)); add a lightweight validation at the start of analyze_lines: verify each item is a tuple of (int, str), ensure line_number values are strictly increasing and unique (previous_line_num < current_line_num) and raise a ValueError (or TypeError for wrong types) with a clear message if the check fails, then call self._analyze_lines(list(lines)) as before; reference analyze_lines and _analyze_lines to locate where to add this check.src/cordon/cli.py (1)
271-327: ⚡ Quick winFactor out shared reporting/output flow used by
analyze_file()andanalyze_stdin().This block mirrors
analyze_file()almost line-for-line. Extracting the common “run analysis + print detailed stats + emit/write content” path into one helper will reduce drift risk and simplify future changes.🤖 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/cordon/cli.py` around lines 271 - 327, The analyze_stdin function duplicates the reporting/output flow found in analyze_file; create a shared helper (e.g., _run_analysis_and_output or similar) and move the common logic—running detailed vs. simple analysis using analyzer.analyze_text_detailed / analyzer.analyze_text, the try/except around analysis, printing the header and detailed stats (total_lines, total_windows, significant_windows, merged_blocks, processing_time, score_distribution), and the final emission via _write_output or stdout—into that helper; then have analyze_stdin and analyze_file call the new helper with appropriate arguments (analyzer, text, detailed, output_path, force) to eliminate the near-identical code paths and keep only small I/O-specific bits in each function.tests/test_cli.py (1)
87-92: ⚡ Quick winAdd behavioral CLI tests for stdin execution path (not just argument parsing).
This test is useful, but the new runtime behavior is in
analyze_stdin()and"-"routing in_main_impl(). Please add tests that mock stdin/analyzer and assert the correct analyzer method is called for detailed vs non-detailed modes.🤖 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 `@tests/test_cli.py` around lines 87 - 92, Add runtime tests that exercise analyze_stdin() and the "-" routing in _main_impl(): set sys.argv to include "-" and replace sys.stdin with an io.StringIO containing test content, then monkeypatch the Analyzer instance methods used by analyze_stdin() (e.g., Analyzer.analyze and Analyzer.analyze_detailed or whatever concrete methods _main_impl() calls) to simple spies/mocks; call _main_impl() (or the public CLI entry that triggers it) and assert the correct analyzer method was invoked for both non-detailed and detailed flags. Ensure you add two cases: one with the detailed flag set (expects the detailed analyze method called) and one without (expects the simple analyze method), and restore monkeypatched stdin/argv after each test.
🤖 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.
Nitpick comments:
In `@src/cordon/cli.py`:
- Around line 271-327: The analyze_stdin function duplicates the
reporting/output flow found in analyze_file; create a shared helper (e.g.,
_run_analysis_and_output or similar) and move the common logic—running detailed
vs. simple analysis using analyzer.analyze_text_detailed /
analyzer.analyze_text, the try/except around analysis, printing the header and
detailed stats (total_lines, total_windows, significant_windows, merged_blocks,
processing_time, score_distribution), and the final emission via _write_output
or stdout—into that helper; then have analyze_stdin and analyze_file call the
new helper with appropriate arguments (analyzer, text, detailed, output_path,
force) to eliminate the near-identical code paths and keep only small
I/O-specific bits in each function.
In `@src/cordon/pipeline.py`:
- Around line 121-130: The public method analyze_lines currently accepts
arbitrary (line_number, line_content) tuples and forwards them to _analyze_lines
after converting to list, which can silently allow duplicate or out-of-order
line numbers (downstream uses dict(lines)); add a lightweight validation at the
start of analyze_lines: verify each item is a tuple of (int, str), ensure
line_number values are strictly increasing and unique (previous_line_num <
current_line_num) and raise a ValueError (or TypeError for wrong types) with a
clear message if the check fails, then call self._analyze_lines(list(lines)) as
before; reference analyze_lines and _analyze_lines to locate where to add this
check.
In `@tests/test_cli.py`:
- Around line 87-92: Add runtime tests that exercise analyze_stdin() and the "-"
routing in _main_impl(): set sys.argv to include "-" and replace sys.stdin with
an io.StringIO containing test content, then monkeypatch the Analyzer instance
methods used by analyze_stdin() (e.g., Analyzer.analyze and
Analyzer.analyze_detailed or whatever concrete methods _main_impl() calls) to
simple spies/mocks; call _main_impl() (or the public CLI entry that triggers it)
and assert the correct analyzer method was invoked for both non-detailed and
detailed flags. Ensure you add two cases: one with the detailed flag set
(expects the detailed analyze method called) and one without (expects the simple
analyze method), and restore monkeypatched stdin/argv after each test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a692319-6e4e-40d0-b7fe-9ebe8bcbfffe
📒 Files selected for processing (4)
src/cordon/cli.pysrc/cordon/pipeline.pytests/test_cli.pytests/test_pipeline_unit.py
calebevans
force-pushed
the
feat/text-api-and-stdin
branch
from
May 23, 2026 22:28
62a8cbf to
c8fb011
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/test_cli.py (3)
189-206:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest mocks wrong method after refactor.
analyze_filenow always callsanalyze_file_detailed, but this test mocksanalyzer.analyze_file.return_value. The test will fail because the actual call toanalyze_file_detailedis not mocked.Proposed fix
def test_force_overwrite(self, tmp_path: Path) -> None: """Test that --force allows overwriting.""" log_file = tmp_path / "test.log" log_file.write_text("line 1\n") output_file = tmp_path / "output.xml" output_file.write_text("existing content") analyzer = MagicMock() - analyzer.analyze_file.return_value = "<anomalies>new</anomalies>" + mock_result = MagicMock() + mock_result.output = "<anomalies>new</anomalies>" + analyzer.analyze_file_detailed.return_value = mock_result analyze_file( log_file, analyzer, detailed=False, output_path=output_file, force=True, ) assert output_file.read_text() == "<anomalies>new</anomalies>"🤖 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 `@tests/test_cli.py` around lines 189 - 206, The test test_force_overwrite is mocking analyzer.analyze_file but analyze_file now always calls analyze_file_detailed; update the test to mock the correct method on the analyzer (set analyzer.analyze_file_detailed.return_value or appropriate side_effect) so the call inside analyze_file returns the expected "<anomalies>new</anomalies>" and the rest of the assertion remains unchanged.
126-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest mocks wrong method after refactor.
analyze_filenow callsanalyze_file_detailedinstead ofanalyze_file, but this test mocksanalyzer.analyze_file.side_effect. The mock won't trigger and the test will fail or behave unexpectedly.Proposed fix
def test_analysis_error_non_detailed( self, capsys: pytest.CaptureFixture[str], tmp_path: Path ) -> None: """Test that analysis errors in non-detailed mode are caught.""" log_file = tmp_path / "test.log" log_file.write_text("line 1\nline 2\n") analyzer = MagicMock() - analyzer.analyze_file.side_effect = ValueError("Bad input") + analyzer.analyze_file_detailed.side_effect = ValueError("Bad input") analyze_file(log_file, analyzer, detailed=False) captured = capsys.readouterr() assert "Error analyzing" in captured.err assert "Bad input" in captured.err🤖 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 `@tests/test_cli.py` around lines 126 - 139, The test currently sets a side effect on analyzer.analyze_file but analyze_file() now delegates to analyzer.analyze_file_detailed; update the mock to set analyzer.analyze_file_detailed.side_effect = ValueError("Bad input") (or otherwise mock the method analyze_file_detailed) so the exception is raised when analyze_file(log_file, analyzer, detailed=False) calls into the refactored code; ensure references to analyzer.analyze_file in this test are replaced with analyzer.analyze_file_detailed to match the new implementation.
208-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest mocks wrong method after refactor.
analyze_filenow always callsanalyze_file_detailed, but this test mocksanalyzer.analyze_file.return_value. The test will fail.Proposed fix
def test_stdout_output(self, capsys: pytest.CaptureFixture[str], tmp_path: Path) -> None: """Test that output is printed to stdout when no output path is given.""" log_file = tmp_path / "test.log" log_file.write_text("line 1\n") analyzer = MagicMock() - analyzer.analyze_file.return_value = "<anomalies>results</anomalies>" + mock_result = MagicMock() + mock_result.output = "<anomalies>results</anomalies>" + analyzer.analyze_file_detailed.return_value = mock_result analyze_file(log_file, analyzer, detailed=False) captured = capsys.readouterr() assert "<anomalies>results</anomalies>" in captured.out🤖 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 `@tests/test_cli.py` around lines 208 - 218, The test test_stdout_output is mocking analyzer.analyze_file but analyze_file now always calls analyzer.analyze_file_detailed; update the test to mock analyzer.analyze_file_detailed (set analyzer.analyze_file_detailed.return_value = "<anomalies>results</anomalies>") so analyze_file() receives the expected result, or adjust the MagicMock to provide both methods, ensuring analyze_file_detailed is the one returning the expected string when analyze_file calls it.
🤖 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.
Outside diff comments:
In `@tests/test_cli.py`:
- Around line 189-206: The test test_force_overwrite is mocking
analyzer.analyze_file but analyze_file now always calls analyze_file_detailed;
update the test to mock the correct method on the analyzer (set
analyzer.analyze_file_detailed.return_value or appropriate side_effect) so the
call inside analyze_file returns the expected "<anomalies>new</anomalies>" and
the rest of the assertion remains unchanged.
- Around line 126-139: The test currently sets a side effect on
analyzer.analyze_file but analyze_file() now delegates to
analyzer.analyze_file_detailed; update the mock to set
analyzer.analyze_file_detailed.side_effect = ValueError("Bad input") (or
otherwise mock the method analyze_file_detailed) so the exception is raised when
analyze_file(log_file, analyzer, detailed=False) calls into the refactored code;
ensure references to analyzer.analyze_file in this test are replaced with
analyzer.analyze_file_detailed to match the new implementation.
- Around line 208-218: The test test_stdout_output is mocking
analyzer.analyze_file but analyze_file now always calls
analyzer.analyze_file_detailed; update the test to mock
analyzer.analyze_file_detailed (set analyzer.analyze_file_detailed.return_value
= "<anomalies>results</anomalies>") so analyze_file() receives the expected
result, or adjust the MagicMock to provide both methods, ensuring
analyze_file_detailed is the one returning the expected string when analyze_file
calls it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3886961b-3bb0-40d0-bbf2-b212bce46aed
📒 Files selected for processing (4)
src/cordon/cli.pysrc/cordon/pipeline.pytests/test_cli.pytests/test_pipeline_unit.py
- Add analyze_text() and analyze_text_detailed() for analyzing raw text without writing to a temp file - Add analyze_lines() for pre-structured line input - Refactor pipeline to share _analyze_lines() between all entry points - Support '-' as filename in CLI to read from stdin - Add tests for text analysis and stdin support Co-authored-by: Cursor <cursoragent@cursor.com>
calebevans
force-pushed
the
feat/text-api-and-stdin
branch
from
May 23, 2026 22:31
c8fb011 to
56436ff
Compare
|
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
analyze_text(text: str) -> strandanalyze_text_detailed(text: str) -> AnalysisResultmethods for analyzing raw log text without writing to a temp fileanalyze_lines(lines: Sequence[tuple[int, str]]) -> AnalysisResultfor users who already have structured line data_analyze_lines()internal method between all entry points, eliminating duplication-as filename in CLI to read from stdin (e.g.,kubectl logs pod | cordon -)analyze_stdin()CLI function for stdin handlingSummary by CodeRabbit
New Features
-as the logfile argument to analyze piped stdin; output can go to stdout or be written to a file (respecting overwrite protection).Refactor
Tests