fix: mark partial streaming scans inconclusive#908
Open
mldangelo-oai wants to merge 3 commits intomainfrom
Open
fix: mark partial streaming scans inconclusive#908mldangelo-oai wants to merge 3 commits intomainfrom
mldangelo-oai wants to merge 3 commits intomainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughPartial streaming analyses are now marked as inconclusive in scan metadata. Helpers set Changes
sequenceDiagram
participant Client
participant ScannerRunner as scan_model_directory_or_file
participant StreamScanner as stream_analyze_file
participant Marker as _mark_streaming_analysis_incomplete
participant Results as ScanResultsStore
Client->>ScannerRunner: request scan (stream://)
ScannerRunner->>StreamScanner: stream_analyze_file(...)
StreamScanner->>StreamScanner: streaming runs, was_complete = False
StreamScanner->>Marker: mark result inconclusive
Marker-->>StreamScanner: result.metadata updated
StreamScanner-->>ScannerRunner: return (ScanResult, was_complete=False)
ScannerRunner->>Results: record marked ScanResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
Performance BenchmarksCompared
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelaudit/utils/file/streaming.py`:
- Around line 229-232: When scan_stream()/scan_bytes() returns a clean
ScanResult for a bounded prefix (scan_result exists, issues is empty,
was_complete is False) the helper currently falls through and returns None;
change the logic so this case is handled: if scan_result is not None and not
was_complete and not issues (i.e. a clean but partial result), call
_mark_streaming_analysis_incomplete(result), compute scanner_success from
scan_result, call result.finish(success=False) and return (result, False) so the
caller receives the inconclusive partial outcome instead of None; update the
branch around scan_result/was_complete/issues to explicitly return this tuple.
In `@tests/test_streaming_scan.py`:
- Around line 113-119: The test must also assert an explicit fail-closed
operational message is present in the file metadata so a disappearing INFO log
doesn't mask the reason; update the assertions after metadata =
result.file_metadata[stream_url] (using result and stream_url) to check that the
metadata includes a human-readable fail-closed message (e.g., an entry like
metadata.get("scan_outcome_message") or any value in metadata.values() contains
the substring "fail" or "failed closed") in addition to the existing checks of
scan_outcome, analysis_incomplete, scan_outcome_reasons, result.success and
determine_exit_code(result).
In `@tests/utils/file/test_streaming_analysis.py`:
- Around line 83-86: The test currently asserts fail-closed metadata but doesn't
verify warning vs error flags; update the test (e.g., in test_streaming_analysis
where `result` is asserted) to also assert `result.has_warnings is True` and
`result.has_errors is False` so bounded-prefix inconclusive cases remain
classified as WARNING-severity and not CRITICAL; add these two assertions
immediately after the existing metadata checks referencing the same `result`
variable.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17ce4e3f-a630-4299-bdda-4473ef7bf8e0
📒 Files selected for processing (5)
CHANGELOG.mdmodelaudit/core.pymodelaudit/utils/file/streaming.pytests/test_streaming_scan.pytests/utils/file/test_streaming_analysis.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
modelaudit/utils/file/streaming.py (1)
221-240:⚠️ Potential issue | 🟠 MajorA clean bounded read can still fall through as
(None, False).If the streamed prefix is partial, the scanner returns no result, and the header heuristics also find nothing, this still returns
None. Thestream://caller treats that as a generic streaming failure instead of the new inconclusive fail-closed outcome.🔧 Proposed fix
- # Create scan result - if scan_result is not None or issues or was_complete: - result = ScanResult(scanner_name="streaming") - scanned = getattr(scan_result, "bytes_scanned", 0) if scan_result is not None else 0 - result.bytes_scanned = scanned or bytes_to_read - result.issues = issues - result.metadata = { - "streaming_analysis": True, - "bytes_analyzed": bytes_to_read, - "analysis_complete": was_complete, - "file_size": file_size, - } - result.metadata.update(metadata) - if not was_complete: - _mark_streaming_analysis_incomplete(result) - scanner_success = bool(getattr(scan_result, "success", True)) if scan_result is not None else True - result.finish(success=was_complete and scanner_success) - return result, was_complete - else: - # Partial analysis with no findings - return None, was_complete + # Create a result for any successful streamed read; reserve `None` for + # transport/setup failures so callers can distinguish fallback from an + # inconclusive partial analysis. + result = ScanResult(scanner_name="streaming") + scanned = getattr(scan_result, "bytes_scanned", 0) if scan_result is not None else 0 + result.bytes_scanned = scanned or bytes_to_read + result.issues = issues + result.metadata = { + "streaming_analysis": True, + "bytes_analyzed": bytes_to_read, + "analysis_complete": was_complete, + "file_size": file_size, + } + result.metadata.update(metadata) + if not was_complete: + _mark_streaming_analysis_incomplete(result) + scanner_success = bool(getattr(scan_result, "success", True)) if scan_result is not None else True + result.finish(success=was_complete and scanner_success) + return result, was_complete🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/utils/file/streaming.py` around lines 221 - 240, When a bounded partial read (bytes_scanned == bytes_to_read) yields no scan_result and no issues, we currently return (None, False) and the caller treats that as a generic streaming failure; instead create and return a ScanResult that marks the analysis as incomplete/inconclusive so the caller can treat it as the fail-closed inconclusive outcome. Modify the branch in streaming_analyze where scan_result is None and issues is empty: instantiate ScanResult(scanner_name="streaming"), set result.bytes_scanned = bytes_to_read, fill result.metadata with "streaming_analysis": True, "bytes_analyzed": bytes_to_read, "analysis_complete": was_complete and add a flag like "inconclusive_fail_closed": True, call _mark_streaming_analysis_incomplete(result) (since was_complete is False), compute scanner_success as before, call result.finish(success=False) to indicate inconclusive/fail-closed, and return result, was_complete instead of None; keep using the existing symbols ScanResult, _mark_streaming_analysis_incomplete, result.finish, and the variables scan_result, bytes_to_read, was_complete, issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_streaming_scan.py`:
- Around line 113-118: The assertions access result.file_metadata[stream_url] as
if it were a plain dict but scan_model_directory_or_file() stores
FileMetadataModel instances; convert or dump the model before subscripting (use
.model_dump() on the FileMetadataModel) so the keys like "scan_outcome",
"analysis_incomplete", "scan_outcome_reasons", and "scan_outcome_message" exist;
update both the block referencing result.file_metadata[stream_url] at the
current assertion group and the similar assertions around lines 147-148 to call
.model_dump() first (or replace direct subscripting with attribute access on
FileMetadataModel) so the fail-closed behavior is actually exercised.
---
Duplicate comments:
In `@modelaudit/utils/file/streaming.py`:
- Around line 221-240: When a bounded partial read (bytes_scanned ==
bytes_to_read) yields no scan_result and no issues, we currently return (None,
False) and the caller treats that as a generic streaming failure; instead create
and return a ScanResult that marks the analysis as incomplete/inconclusive so
the caller can treat it as the fail-closed inconclusive outcome. Modify the
branch in streaming_analyze where scan_result is None and issues is empty:
instantiate ScanResult(scanner_name="streaming"), set result.bytes_scanned =
bytes_to_read, fill result.metadata with "streaming_analysis": True,
"bytes_analyzed": bytes_to_read, "analysis_complete": was_complete and add a
flag like "inconclusive_fail_closed": True, call
_mark_streaming_analysis_incomplete(result) (since was_complete is False),
compute scanner_success as before, call result.finish(success=False) to indicate
inconclusive/fail-closed, and return result, was_complete instead of None; keep
using the existing symbols ScanResult, _mark_streaming_analysis_incomplete,
result.finish, and the variables scan_result, bytes_to_read, was_complete,
issues.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc7dce83-59c1-4390-a30e-55fb5b9305f2
📒 Files selected for processing (4)
modelaudit/core.pymodelaudit/utils/file/streaming.pytests/test_streaming_scan.pytests/utils/file/test_streaming_analysis.py
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
Validation
Summary by CodeRabbit
Bug Fixes
Tests