Feat(#37): Gemini 조건부 호출 및 하이브리드 텍스트 분석 파이프라인 구현 (2/3) - #59
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe PR replaces legacy text-analysis orchestration with conditional Stacking/Gemini routing. It adds threshold selection, fail-safe engine handling, pipeline integration, RabbitMQ metadata, validation artifacts, tests, and documentation. ChangesHybrid stacking and Gemini analysis
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SmishingAnalysisService
participant HybridTextAnalyzer
participant StackingAnalyzer
participant ConditionalGeminiPolicy
participant GeminiAnalyzer
participant RabbitMQResultFactory
Client->>SmishingAnalysisService: submit message text
SmishingAnalysisService->>HybridTextAnalyzer: analyze text
HybridTextAnalyzer->>StackingAnalyzer: run Stacking analysis
StackingAnalyzer-->>ConditionalGeminiPolicy: return probability and availability
ConditionalGeminiPolicy-->>HybridTextAnalyzer: return routing decision
HybridTextAnalyzer->>GeminiAnalyzer: run Gemini when required
GeminiAnalyzer-->>HybridTextAnalyzer: return result or normalized failure
HybridTextAnalyzer-->>SmishingAnalysisService: return hybrid analysis metadata
SmishingAnalysisService->>RabbitMQResultFactory: build text analysis detail
RabbitMQResultFactory-->>Client: publish analysis result metadata
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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.
Actionable comments posted: 13
🧹 Nitpick comments (11)
data_science/SMSModel/run_hybrid_threshold_selection.py (2)
101-176: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDocument the trust boundary of the checksum check.
The digest comes from
metadata.json, which sits in the same directory asmodel.joblib. Anyone who can replace the artifact can also replace the expected digest. The check protects against corruption and stale artifacts. It does not protect against tampering, becausejoblib.loadunpickles and can execute arbitrary code. Add a short comment that states this, so a later reader does not treat the check as a security control.🤖 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 `@data_science/SMSModel/run_hybrid_threshold_selection.py` around lines 101 - 176, The checksum validation in _load_stacking_classifier only detects corruption or stale artifacts, not tampering, because model_sha256 is read from the colocated metadata.json and joblib.load can execute pickle code. Add a short comment immediately before the checksum comparison or deserialization documenting this trust boundary and explicitly stating that the check is not a security control.Source: Linters/SAST tools
441-464: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the Gemini phishing score constant.
The value
40is hardcoded at line 457 and at line 527, and it is also the default ofgemini_phishing_scoreinselect_hybrid_thresholds. If the SAFE/SUSPICIOUS boundary changes, the report can disagree with the value used for selection. Define one module-level constant and use it in both places.♻️ Proposed refactor
+GEMINI_PHISHING_SCORE = 40 + def _write_policy_report( selection: HybridThresholdSelection, ) -> None: @@ - "gemini_phishing_score": 40, + "gemini_phishing_score": GEMINI_PHISHING_SCORE,target_recall=target_recall, - - gemini_phishing_score=40, + gemini_phishing_score=GEMINI_PHISHING_SCORE, )🤖 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 `@data_science/SMSModel/run_hybrid_threshold_selection.py` around lines 441 - 464, Define a single module-level constant for the Gemini phishing score, then replace the hardcoded 40 in _write_policy_report and the other usage near line 527, and use the same constant as the default gemini_phishing_score in select_hybrid_thresholds so selection and reporting remain consistent.data_science/SMSModel/modeling/hybrid_thresholds.py (1)
185-236: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftReduce the search cost of the threshold grid.
candidatescontains101 + nunique values. The nested loop evaluates about(101 + n)^2 / 2pairs, and each accepted pair calls three scikit-learn metric functions overnsamples. The cost grows as O(n^3). With a few thousand validation rows the selection run becomes impractical. The committed cache holds only 17 rows, so the cost is hidden today.Two options:
- Restrict the probability candidates to quantiles (for example 200 evenly spaced quantiles) instead of every observed probability.
- Compute the confusion counts with vectorized NumPy over sorted probabilities and derive recall, precision, and F2 directly, instead of calling
recall_score,precision_score, andfbeta_scoreinside the loop.♻️ Example: bound the candidate grid
# 고정 간격 후보와 실제 확률값을 함께 사용 + MAX_PROBABILITY_CANDIDATES = 200 + + probability_candidates = np.quantile( + probabilities, + np.linspace( + 0.0, + 1.0, + MAX_PROBABILITY_CANDIDATES, + ), + ) + candidates = np.unique( np.concatenate( [ np.linspace( 0.0, 1.0, 101, ), - probabilities, + probability_candidates, ] ) )🤖 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 `@data_science/SMSModel/modeling/hybrid_thresholds.py` around lines 185 - 236, Reduce the threshold-search cost in the candidate generation and nested loops by bounding observed probability candidates to a fixed-size quantile grid (for example, at most 200 evenly spaced quantiles) while retaining the fixed 0.0–1.0 boundaries and uniqueness. Preserve valid normal_max/phishing_min ordering and the existing metric-based selection behavior.tests/analysis/test_execution.py (1)
35-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding the remaining hybrid fields to the builder.
_text_analysisomitsdecision_source,routing_reason,fallback_applied, andself_model.confidence. The fixtures intests/infrastructure/rabbitmq/test_result_factory.pyandtests/infrastructure/rabbitmq/test_consumer.pyinclude them.classify_executiondoes not read those keys today, so no test fails. Adding them keeps one shared shape for the hybrid text response and protects the classifier tests if the classifier later reads routing metadata.🤖 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/analysis/test_execution.py` around lines 35 - 56, Extend the _text_analysis test-data builder to include decision_source, routing_reason, and fallback_applied in the result payload, plus confidence under self_model, using the shared hybrid response values and defaults established by the RabbitMQ fixtures. Preserve the existing score, error, and Gemini-related fields while keeping the returned schema consistent for classifier tests.tests/data_science/SMSModel/modeling/test_hybrid_thresholds.py (1)
105-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the unreachable-target branch.
The parametrized cases cover the
ValueErrorpaths in_validate_inputs. TheRuntimeErroratdata_science/SMSModel/modeling/hybrid_thresholds.pylines 310-314 has no test. That branch decides whether an unreachable recall target stops the workflow or silently returns a weak policy. Add a case where no threshold pair reachestarget_recall, for example labels that Gemini and stacking both misclassify.The label-set check at lines 121-130 is also untested. A single-label array is a cheap case to add.
🤖 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/data_science/SMSModel/modeling/test_hybrid_thresholds.py` around lines 105 - 152, Add parametrized coverage in test_rejects_invalid_inputs for a single-label labels array to exercise the label-set validation, and add a separate test for select_hybrid_thresholds where Gemini and stacking both misclassify so no threshold pair reaches target_recall and the unreachable-target RuntimeError is asserted. Use the existing test helpers and invocation style.tests/infrastructure/rabbitmq/test_result_factory.py (1)
144-153: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for the fallback and unavailable methods.
These assertions cover the
STACKING_GEMINIpath only.TextAnalysisMethodinapp/infrastructure/rabbitmq/schemas.pyalso definesSTACKING,STACKING_FALLBACK, andUNAVAILABLE. The PR objectives describe Gemini timeouts, API errors, and rate limits that fall back to Stacking, and anALL_TEXT_ENGINES_UNAVAILABLEoutcome. The mapping from text-analysis metadata to those three enum values is untested here, and it is the metadata that downstream consumers read.Add cases for a fallback result (
fallback_applied: True) and for an all-engines-unavailable result.🤖 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/infrastructure/rabbitmq/test_result_factory.py` around lines 144 - 153, Add test cases alongside the existing STACKING_GEMINI assertions to cover metadata mapping for a fallback result with fallback_applied=True, expecting TextAnalysisMethod.STACKING_FALLBACK, and an all-engines-unavailable result, expecting TextAnalysisMethod.UNAVAILABLE and the corresponding outcome metadata. Reuse the existing event factory and assertion style so downstream-readable fields are verified for both paths.tests/analysis/text/test_hybrid_analyzer.py (1)
171-192: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that the upstream exception text does not leak into the result.
The test uses the message
"secret upstream detail"to model sensitive upstream detail. It does not verify that the message is absent from the returned payload. Add an explicit assertion so a future change that surfacesstr(exception)fails this test.💚 Proposed addition
assert ( result["gemini"]["error_message"] == "GEMINI_ANALYZER_FAILED" ) + assert "secret upstream detail" not in str(result)🤖 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/analysis/text/test_hybrid_analyzer.py` around lines 171 - 192, Update test_uses_stacking_when_gemini_raises to assert that the returned result contains no occurrence of the sensitive text "secret upstream detail", especially within result["gemini"]["error_message"]. Preserve the existing fallback and sanitized error-code assertions.app/analysis/text/hybrid_analyzer.py (2)
106-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilence the BLE001 warnings for the intentional fail-safe catches.
Both broad catches are deliberate. They normalize engine failures instead of propagating them.
app/analysis/text/stacking_analyzer.pyalready marks the same pattern with# noqa: BLE001. Apply the same marker here for consistency with the linter configuration.♻️ Proposed change
- except Exception as exception: + except Exception as exception: # noqa: BLE001Also applies to: 172-172
🤖 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 `@app/analysis/text/hybrid_analyzer.py` at line 106, Add the # noqa: BLE001 marker to both intentional broad Exception handlers in the relevant analyzer methods, including the catches around the lines represented by “except Exception as exception.” Match the existing suppression pattern used in stacking_analyzer.py without changing the fail-safe behavior.Source: Linters/SAST tools
269-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
routing.decision.valueinstead of the hardcoded enum.This branch runs only when
stacking_analysis["is_available"]is falsy.ConditionalGeminiPolicy.routealready returnsGEMINI_FALLBACKfor that input, so the hardcoded value duplicates the policy decision. The adjacentrouting_reasonalready reads fromrouting. Reading both fields fromroutingkeeps the metadata consistent if the policy adds a new unavailable-model decision later.♻️ Proposed change
- "routing_decision": ( - HybridRoutingDecision - .GEMINI_FALLBACK - .value - ), + "routing_decision": ( + routing.decision.value + ),Remove the now-unused
HybridRoutingDecisionimport if no other reference remains.🤖 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 `@app/analysis/text/hybrid_analyzer.py` around lines 269 - 273, Update the unavailable-model branch in the hybrid analysis flow to set routing_decision from routing.decision.value, matching the adjacent routing_reason metadata and preserving the policy’s returned decision. Remove the HybridRoutingDecision import if no other references remain.tests/analysis/test_hybrid_policy.py (1)
82-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the two uncovered guard branches and the threshold boundaries.
ConditionalGeminiPolicy.routehas two fail-safe branches that no test exercises:STACKING_PROBABILITY_UNAVAILABLE(line 92 ofapp/analysis/hybrid_policy.py) andINVALID_STACKING_PROBABILITY(line 103). Both prevent a malformed stacking result from being treated as normal. The boundary valuesnormal_maxandphishing_minare also untested, and the comparisons use<=and>=.💚 Proposed additional tests
`@pytest.mark.parametrize`( "probability", [None, "0.5", float("nan"), 1.5, -0.1], ) def test_falls_back_to_gemini_for_unusable_probability( policy: ConditionalGeminiPolicy, probability, ) -> None: result = policy.route( { "engine": "stacking", "is_available": True, "result": {"risk_probability": probability}, } ) assert ( result.decision == HybridRoutingDecision.GEMINI_FALLBACK ) assert result.should_call_gemini is True `@pytest.mark.parametrize`( ("probability", "expected"), [ (0.2, HybridRoutingDecision.SELF_MODEL_NORMAL), (0.8, HybridRoutingDecision.SELF_MODEL_PHISHING), ], ) def test_threshold_boundaries_are_inclusive( policy: ConditionalGeminiPolicy, probability: float, expected: HybridRoutingDecision, ) -> None: result = policy.route( build_stacking_result(probability) ) assert result.decision == expected assert result.should_call_gemini is False🤖 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/analysis/test_hybrid_policy.py` around lines 82 - 119, Add tests in tests/analysis/test_hybrid_policy.py covering ConditionalGeminiPolicy.route’s unavailable and invalid stacking-probability fallback branches with None, nonnumeric, NaN, and out-of-range values, asserting GEMINI_FALLBACK and should_call_gemini=True. Also add boundary tests for HybridThresholds using probabilities equal to normal_max and phishing_min, asserting the inclusive SELF_MODEL_NORMAL and SELF_MODEL_PHISHING decisions without Gemini.tests/analysis/test_service.py (1)
120-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact final score for the Gemini escalation path.
assert result.final_score >= 40passes for a wide range of values. It does not distinguish the Gemini score of 90 from the blended text-track score thatRiskScoringEnginecurrently computes fromself_model_score=50andselected_score=90. See the related comment onapp/analysis/service.pyLines 331-336.Assert the exact expected score and the text contribution. A precise assertion documents which score the pipeline applies and fails if the blending semantics change.
💚 Proposed change
assert result.status == "SUCCESS" assert result.text_analysis["decision_source"] == "GEMINI" assert result.text_analysis["gemini_available"] is True - assert result.final_score >= 40 + # 텍스트 트랙이 어떤 점수를 적용했는지 명시적으로 고정 + assert result.contribution_breakdown.llm == <expected> + assert result.final_score == <expected>🤖 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/analysis/test_service.py` around lines 120 - 123, Update the Gemini escalation assertions in the test to verify the exact expected final_score produced by the current blending logic, and assert the corresponding text contribution as described by RiskScoringEngine. Replace the broad lower-bound check while preserving the existing status, decision_source, and gemini_available assertions.
🤖 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 `@app/analysis/service.py`:
- Around line 331-336: The service text-track call at app/analysis/service.py
lines 331-336 should pass only the HybridTextAnalyzer-selected score as the
single text signal, remove the raw naive_bayes_score=self_model_score argument,
and derive llm_available from whether a selected score exists. Update
tests/analysis/test_service.py lines 120-123 to assert the exact
result.final_score and result.contribution_breakdown.llm values, pinning the
applied selected text score.
- Around line 136-146: Update the asyncio.gather flow around text_task and
url_task to collect both task outcomes and handle exceptions explicitly. When
either task fails, cancel the sibling task, await its completion, and preserve
the existing fail-safe ERROR response; ensure both task results or exceptions
are retrieved so no background work or unhandled task warning remains.
In `@app/analysis/text/hybrid_analyzer.py`:
- Around line 104-105: Update the async analyze flow around
self.stacking_analyzer in HybridAnalyzer.analyze to execute the synchronous
callable via asyncio.to_thread, adding the asyncio import. Preserve the existing
arguments, result handling, and exception behavior while preventing blocking of
the event loop.
In `@app/core/config.py`:
- Around line 20-32: Update the default values for
STACKING_NORMAL_PROBABILITY_MAX and STACKING_PHISHING_PROBABILITY_MIN to
conservative, non-degenerate thresholds such as 0.1 and 0.9, while preserving
their existing 0–1 validation. Ensure the defaults no longer make nearly every
probability fall into the uncertain band.
In `@app/infrastructure/rabbitmq/result_factory.py`:
- Around line 163-165: Update the result mapping around selfModelConfidence and
add a _confidence helper next to _integer_score. Have the helper reject
booleans, non-numeric values, and NaN as None, while converting valid numbers to
float and clamping finite values to the 0.0–1.0 range before assigning
selfModelConfidence.
In `@data_science/SMSModel/artifacts/stacking/gemini_validation_predictions.json`:
- Around line 1-108: Document that the committed gemini_validation_predictions
cache is intentionally partial because of Gemini quota limits, including that
unavailable entries prevent threshold selection and are not a regression. Add
this note to the relevant PR or documentation while leaving the
fingerprint-and-score cache unchanged.
In `@data_science/SMSModel/run_hybrid_threshold_selection.py`:
- Around line 351-370: Update the score validation in the available calculation
to explicitly reject bool values while continuing to accept only integer scores
in the existing 0–100 range; preserve the current integer risk_score contract
and downstream _ordered_gemini_scores behavior.
In `@docs/PII_MASKING.md`:
- Around line 24-26: Update the deployment sequence in the SafeFam_BE/SafeFam_AI
token-format migration instructions to deploy SafeFam_AI before SafeFam_BE.
Alternatively, require both token formats to be supported by SafeFam_AI before
deploying the new-emitting SafeFam_BE version.
In `@docs/SCORING_PIPELINE_CHANGES.md`:
- Around line 109-120: Update docs/SCORING_PIPELINE_CHANGES.md lines 109-120 to
distinguish final-grade thresholds (0–39 LOW, 40–69 MEDIUM, 70–100 HIGH) from
the separate thresholds that route uncertain results to Gemini, documenting the
selected hybrid policy without conflating the two. Update docs/TRAINING_FLOW.md
lines 74-82 to include Recall, F2, Gemini call rate, fingerprint caching, policy
metadata, and the provisional validation state.
- Around line 123-136: Update the “테스트 현황” section to report the complete
validation result, including 434 passed and 3 deselected tests, the exact test
scope, and the Gemini quota limitation. Mark the final thresholds and Gemini
call rate as provisional until those limitations are resolved, while retaining
the existing list of relevant tests and out-of-scope files.
- Around line 5-17: Update the documentation to match the current
Stacking/Gemini contract: in docs/SCORING_PIPELINE_CHANGES.md lines 5-17,
replace the Naive Bayes-first and SAFE-only routing diagram with high-confidence
and uncertain-result routing; in docs/PII_MASKING.md lines 18-20, identify the
Stacking hybrid analyzer and Gemini uncertainty path; and in
docs/TRAINING_FLOW.md line 172, replace Claude and medium-only escalation
terminology with Gemini and the current routing policy.
- Around line 102-105: Update the scoring documentation around the VirusTotal
formula to state that raw_score is normalized to 0–1, while
app/infrastructure/rabbitmq/result_factory.py:_url_score converts it to 0–100
for external output. Add boundary examples for raw scores 0 and 1.0, plus the
zero-engines case, and clarify that callers must not apply the conversion twice.
In `@docs/TRAINING_FLOW.md`:
- Around line 113-118: The documentation for generate_voice_data.py must clarify
that its precomputed 68/16/16 split assignment is informational because
_leak_free_split() reassigns splits; alternatively remove the precomputed split
claim. Explicitly state whether each 15% allocation is calculated from the
original dataset or from the remaining subset, consistently in both affected
sections.
---
Nitpick comments:
In `@app/analysis/text/hybrid_analyzer.py`:
- Line 106: Add the # noqa: BLE001 marker to both intentional broad Exception
handlers in the relevant analyzer methods, including the catches around the
lines represented by “except Exception as exception.” Match the existing
suppression pattern used in stacking_analyzer.py without changing the fail-safe
behavior.
- Around line 269-273: Update the unavailable-model branch in the hybrid
analysis flow to set routing_decision from routing.decision.value, matching the
adjacent routing_reason metadata and preserving the policy’s returned decision.
Remove the HybridRoutingDecision import if no other references remain.
In `@data_science/SMSModel/modeling/hybrid_thresholds.py`:
- Around line 185-236: Reduce the threshold-search cost in the candidate
generation and nested loops by bounding observed probability candidates to a
fixed-size quantile grid (for example, at most 200 evenly spaced quantiles)
while retaining the fixed 0.0–1.0 boundaries and uniqueness. Preserve valid
normal_max/phishing_min ordering and the existing metric-based selection
behavior.
In `@data_science/SMSModel/run_hybrid_threshold_selection.py`:
- Around line 101-176: The checksum validation in _load_stacking_classifier only
detects corruption or stale artifacts, not tampering, because model_sha256 is
read from the colocated metadata.json and joblib.load can execute pickle code.
Add a short comment immediately before the checksum comparison or
deserialization documenting this trust boundary and explicitly stating that the
check is not a security control.
- Around line 441-464: Define a single module-level constant for the Gemini
phishing score, then replace the hardcoded 40 in _write_policy_report and the
other usage near line 527, and use the same constant as the default
gemini_phishing_score in select_hybrid_thresholds so selection and reporting
remain consistent.
In `@tests/analysis/test_execution.py`:
- Around line 35-56: Extend the _text_analysis test-data builder to include
decision_source, routing_reason, and fallback_applied in the result payload,
plus confidence under self_model, using the shared hybrid response values and
defaults established by the RabbitMQ fixtures. Preserve the existing score,
error, and Gemini-related fields while keeping the returned schema consistent
for classifier tests.
In `@tests/analysis/test_hybrid_policy.py`:
- Around line 82-119: Add tests in tests/analysis/test_hybrid_policy.py covering
ConditionalGeminiPolicy.route’s unavailable and invalid stacking-probability
fallback branches with None, nonnumeric, NaN, and out-of-range values, asserting
GEMINI_FALLBACK and should_call_gemini=True. Also add boundary tests for
HybridThresholds using probabilities equal to normal_max and phishing_min,
asserting the inclusive SELF_MODEL_NORMAL and SELF_MODEL_PHISHING decisions
without Gemini.
In `@tests/analysis/test_service.py`:
- Around line 120-123: Update the Gemini escalation assertions in the test to
verify the exact expected final_score produced by the current blending logic,
and assert the corresponding text contribution as described by
RiskScoringEngine. Replace the broad lower-bound check while preserving the
existing status, decision_source, and gemini_available assertions.
In `@tests/analysis/text/test_hybrid_analyzer.py`:
- Around line 171-192: Update test_uses_stacking_when_gemini_raises to assert
that the returned result contains no occurrence of the sensitive text "secret
upstream detail", especially within result["gemini"]["error_message"]. Preserve
the existing fallback and sanitized error-code assertions.
In `@tests/data_science/SMSModel/modeling/test_hybrid_thresholds.py`:
- Around line 105-152: Add parametrized coverage in test_rejects_invalid_inputs
for a single-label labels array to exercise the label-set validation, and add a
separate test for select_hybrid_thresholds where Gemini and stacking both
misclassify so no threshold pair reaches target_recall and the
unreachable-target RuntimeError is asserted. Use the existing test helpers and
invocation style.
In `@tests/infrastructure/rabbitmq/test_result_factory.py`:
- Around line 144-153: Add test cases alongside the existing STACKING_GEMINI
assertions to cover metadata mapping for a fallback result with
fallback_applied=True, expecting TextAnalysisMethod.STACKING_FALLBACK, and an
all-engines-unavailable result, expecting TextAnalysisMethod.UNAVAILABLE and the
corresponding outcome metadata. Reuse the existing event factory and assertion
style so downstream-readable fields are verified for both paths.
🪄 Autofix
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: 5bba9692-de3c-4670-bef0-161d9095a7e9
📒 Files selected for processing (22)
app/analysis/execution.pyapp/analysis/hybrid_policy.pyapp/analysis/service.pyapp/analysis/text/hybrid_analyzer.pyapp/core/config.pyapp/infrastructure/rabbitmq/result_factory.pyapp/infrastructure/rabbitmq/schemas.pydata_science/SMSModel/artifacts/stacking/gemini_validation_predictions.jsondata_science/SMSModel/modeling/hybrid_thresholds.pydata_science/SMSModel/run_hybrid_threshold_selection.pydocs/PII_MASKING.mddocs/SCORING_PIPELINE_CHANGES.mddocs/TRAINING_FLOW.mdtests/analysis/test_execution.pytests/analysis/test_hybrid_policy.pytests/analysis/test_router.pytests/analysis/test_service.pytests/analysis/text/test_hybrid_analyzer.pytests/core/test_config.pytests/data_science/SMSModel/modeling/test_hybrid_thresholds.pytests/infrastructure/rabbitmq/test_consumer.pytests/infrastructure/rabbitmq/test_result_factory.py
| { | ||
| "predictions": [ | ||
| { | ||
| "available": false, | ||
| "error_code": "GEMINI_VALIDATION_FAILED", | ||
| "risk_score": null, | ||
| "text_fingerprint": "289b21983f4d981cd429323483365d60e967d089656d930db86180446bbc44f0" | ||
| }, | ||
| { | ||
| "available": true, | ||
| "error_code": null, | ||
| "risk_score": 92, | ||
| "text_fingerprint": "2ea8346732eddecebb3d5b0af2dadfe19eac847d63d1a54616e3c9780c0d0a9b" | ||
| }, | ||
| { | ||
| "available": true, | ||
| "error_code": null, | ||
| "risk_score": 5, | ||
| "text_fingerprint": "301f545f30fe3a4b2ea8bf0be80d564782893f0137f7fc10645244a3d82eb833" | ||
| }, | ||
| { | ||
| "available": true, | ||
| "error_code": null, | ||
| "risk_score": 92, | ||
| "text_fingerprint": "3256ba9d4ad81438c3a78ad2d9d9124a2f6db7d2ba33c934f2093f1ea2b55742" | ||
| }, | ||
| { | ||
| "available": true, | ||
| "error_code": null, | ||
| "risk_score": 92, | ||
| "text_fingerprint": "354a654dd179372d1c17ae688d9cffbc5ff4afaddf944bffe01a178152368903" | ||
| }, | ||
| { | ||
| "available": false, | ||
| "error_code": "GEMINI_VALIDATION_FAILED", | ||
| "risk_score": null, | ||
| "text_fingerprint": "3ee443e030ab5aff20c11eb1d2a3857f363d8fe4a989e99448b0e12e9069e216" | ||
| }, | ||
| { | ||
| "available": true, | ||
| "error_code": null, | ||
| "risk_score": 85, | ||
| "text_fingerprint": "4142fcb037bf6abb39e3c8d499beba8b3f08276d63f3f041911c1ebd7a574222" | ||
| }, | ||
| { | ||
| "available": true, | ||
| "error_code": null, | ||
| "risk_score": 5, | ||
| "text_fingerprint": "5580c99edd3218684721ed377d478d39496755e1956427c69ec48ccedee48db8" | ||
| }, | ||
| { | ||
| "available": true, | ||
| "error_code": null, | ||
| "risk_score": 95, | ||
| "text_fingerprint": "638d116d425486fdd6c6d02756dbc9f8c32288aa05c6129adbcd6ef8ee8e127c" | ||
| }, | ||
| { | ||
| "available": true, | ||
| "error_code": null, | ||
| "risk_score": 90, | ||
| "text_fingerprint": "6954d76bffa4df64d32eece72440e5da18f1f6546b2f98ec65b344bbdfac4056" | ||
| }, | ||
| { | ||
| "available": true, | ||
| "error_code": null, | ||
| "risk_score": 10, | ||
| "text_fingerprint": "7243157b2c8f9fab1664cd2b01270de2d106b872c1620e2402a1c9329c2ba243" | ||
| }, | ||
| { | ||
| "available": true, | ||
| "error_code": null, | ||
| "risk_score": 5, | ||
| "text_fingerprint": "828b133c682a90cf80dc66ddb563f0bad1add03d628f6ebb70e813269eba681f" | ||
| }, | ||
| { | ||
| "available": true, | ||
| "error_code": null, | ||
| "risk_score": 75, | ||
| "text_fingerprint": "af129827c3283f2218baef8752bf17dc48159f820b6793985bfcd7c83213f83b" | ||
| }, | ||
| { | ||
| "available": false, | ||
| "error_code": "GEMINI_VALIDATION_FAILED", | ||
| "risk_score": null, | ||
| "text_fingerprint": "d032c79c99bf3ca9213f7263f47f32cfd8526e911d5a849518b237e4517ad7ba" | ||
| }, | ||
| { | ||
| "available": false, | ||
| "error_code": "GEMINI_VALIDATION_FAILED", | ||
| "risk_score": null, | ||
| "text_fingerprint": "d12d5c8c893c7770de85d622bb84933abd4c5b8dd6da76a497f7db729c9cb1ca" | ||
| }, | ||
| { | ||
| "available": true, | ||
| "error_code": null, | ||
| "risk_score": 92, | ||
| "text_fingerprint": "eb8f6d7e5925e119738ba91c9b0ed18f2ecb8d9b2a3a3bff7ae366435b3681d4" | ||
| }, | ||
| { | ||
| "available": true, | ||
| "error_code": null, | ||
| "risk_score": 90, | ||
| "text_fingerprint": "ee03ab85bde5b1e60c128fbd34eabecc0c854d63f30131db8df9f8071ba28de8" | ||
| } | ||
| ], | ||
| "schema_version": 1, | ||
| "updated_at": "2026-08-11T05:19:14.067891+00:00" | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The committed cache cannot produce thresholds in its current state.
The file holds 17 entries, and 4 of them have "available": false. _ordered_gemini_scores in data_science/SMSModel/run_hybrid_threshold_selection.py raises RuntimeError when any expected fingerprint is not available, and it also raises when the validation split contains fingerprints that are absent here. A run of run_hybrid_threshold_selection.py against the full validation split therefore fails before selection.
This matches the PR statement that validation is incomplete because of Gemini quota limits. The file is safe to commit, because it stores only fingerprints and scores. Record the partial state in the PR or in the docs so a later run is not treated as a regression.
🤖 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 `@data_science/SMSModel/artifacts/stacking/gemini_validation_predictions.json`
around lines 1 - 108, Document that the committed gemini_validation_predictions
cache is intentionally partial because of Gemini quota limits, including that
unavailable entries prevent threshold selection and are not a regression. Add
this note to the relevant PR or documentation while leaving the
fingerprint-and-score cache unchanged.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 13
🧹 Nitpick comments (11)
data_science/SMSModel/run_hybrid_threshold_selection.py (2)
101-176: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDocument the trust boundary of the checksum check.
The digest comes from
metadata.json, which sits in the same directory asmodel.joblib. Anyone who can replace the artifact can also replace the expected digest. The check protects against corruption and stale artifacts. It does not protect against tampering, becausejoblib.loadunpickles and can execute arbitrary code. Add a short comment that states this, so a later reader does not treat the check as a security control.🤖 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 `@data_science/SMSModel/run_hybrid_threshold_selection.py` around lines 101 - 176, The checksum validation in _load_stacking_classifier only detects corruption or stale artifacts, not tampering, because model_sha256 is read from the colocated metadata.json and joblib.load can execute pickle code. Add a short comment immediately before the checksum comparison or deserialization documenting this trust boundary and explicitly stating that the check is not a security control.Source: Linters/SAST tools
441-464: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the Gemini phishing score constant.
The value
40is hardcoded at line 457 and at line 527, and it is also the default ofgemini_phishing_scoreinselect_hybrid_thresholds. If the SAFE/SUSPICIOUS boundary changes, the report can disagree with the value used for selection. Define one module-level constant and use it in both places.♻️ Proposed refactor
+GEMINI_PHISHING_SCORE = 40 + def _write_policy_report( selection: HybridThresholdSelection, ) -> None: @@ - "gemini_phishing_score": 40, + "gemini_phishing_score": GEMINI_PHISHING_SCORE,target_recall=target_recall, - - gemini_phishing_score=40, + gemini_phishing_score=GEMINI_PHISHING_SCORE, )🤖 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 `@data_science/SMSModel/run_hybrid_threshold_selection.py` around lines 441 - 464, Define a single module-level constant for the Gemini phishing score, then replace the hardcoded 40 in _write_policy_report and the other usage near line 527, and use the same constant as the default gemini_phishing_score in select_hybrid_thresholds so selection and reporting remain consistent.data_science/SMSModel/modeling/hybrid_thresholds.py (1)
185-236: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftReduce the search cost of the threshold grid.
candidatescontains101 + nunique values. The nested loop evaluates about(101 + n)^2 / 2pairs, and each accepted pair calls three scikit-learn metric functions overnsamples. The cost grows as O(n^3). With a few thousand validation rows the selection run becomes impractical. The committed cache holds only 17 rows, so the cost is hidden today.Two options:
- Restrict the probability candidates to quantiles (for example 200 evenly spaced quantiles) instead of every observed probability.
- Compute the confusion counts with vectorized NumPy over sorted probabilities and derive recall, precision, and F2 directly, instead of calling
recall_score,precision_score, andfbeta_scoreinside the loop.♻️ Example: bound the candidate grid
# 고정 간격 후보와 실제 확률값을 함께 사용 + MAX_PROBABILITY_CANDIDATES = 200 + + probability_candidates = np.quantile( + probabilities, + np.linspace( + 0.0, + 1.0, + MAX_PROBABILITY_CANDIDATES, + ), + ) + candidates = np.unique( np.concatenate( [ np.linspace( 0.0, 1.0, 101, ), - probabilities, + probability_candidates, ] ) )🤖 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 `@data_science/SMSModel/modeling/hybrid_thresholds.py` around lines 185 - 236, Reduce the threshold-search cost in the candidate generation and nested loops by bounding observed probability candidates to a fixed-size quantile grid (for example, at most 200 evenly spaced quantiles) while retaining the fixed 0.0–1.0 boundaries and uniqueness. Preserve valid normal_max/phishing_min ordering and the existing metric-based selection behavior.tests/analysis/test_execution.py (1)
35-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding the remaining hybrid fields to the builder.
_text_analysisomitsdecision_source,routing_reason,fallback_applied, andself_model.confidence. The fixtures intests/infrastructure/rabbitmq/test_result_factory.pyandtests/infrastructure/rabbitmq/test_consumer.pyinclude them.classify_executiondoes not read those keys today, so no test fails. Adding them keeps one shared shape for the hybrid text response and protects the classifier tests if the classifier later reads routing metadata.🤖 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/analysis/test_execution.py` around lines 35 - 56, Extend the _text_analysis test-data builder to include decision_source, routing_reason, and fallback_applied in the result payload, plus confidence under self_model, using the shared hybrid response values and defaults established by the RabbitMQ fixtures. Preserve the existing score, error, and Gemini-related fields while keeping the returned schema consistent for classifier tests.tests/data_science/SMSModel/modeling/test_hybrid_thresholds.py (1)
105-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the unreachable-target branch.
The parametrized cases cover the
ValueErrorpaths in_validate_inputs. TheRuntimeErroratdata_science/SMSModel/modeling/hybrid_thresholds.pylines 310-314 has no test. That branch decides whether an unreachable recall target stops the workflow or silently returns a weak policy. Add a case where no threshold pair reachestarget_recall, for example labels that Gemini and stacking both misclassify.The label-set check at lines 121-130 is also untested. A single-label array is a cheap case to add.
🤖 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/data_science/SMSModel/modeling/test_hybrid_thresholds.py` around lines 105 - 152, Add parametrized coverage in test_rejects_invalid_inputs for a single-label labels array to exercise the label-set validation, and add a separate test for select_hybrid_thresholds where Gemini and stacking both misclassify so no threshold pair reaches target_recall and the unreachable-target RuntimeError is asserted. Use the existing test helpers and invocation style.tests/infrastructure/rabbitmq/test_result_factory.py (1)
144-153: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for the fallback and unavailable methods.
These assertions cover the
STACKING_GEMINIpath only.TextAnalysisMethodinapp/infrastructure/rabbitmq/schemas.pyalso definesSTACKING,STACKING_FALLBACK, andUNAVAILABLE. The PR objectives describe Gemini timeouts, API errors, and rate limits that fall back to Stacking, and anALL_TEXT_ENGINES_UNAVAILABLEoutcome. The mapping from text-analysis metadata to those three enum values is untested here, and it is the metadata that downstream consumers read.Add cases for a fallback result (
fallback_applied: True) and for an all-engines-unavailable result.🤖 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/infrastructure/rabbitmq/test_result_factory.py` around lines 144 - 153, Add test cases alongside the existing STACKING_GEMINI assertions to cover metadata mapping for a fallback result with fallback_applied=True, expecting TextAnalysisMethod.STACKING_FALLBACK, and an all-engines-unavailable result, expecting TextAnalysisMethod.UNAVAILABLE and the corresponding outcome metadata. Reuse the existing event factory and assertion style so downstream-readable fields are verified for both paths.tests/analysis/text/test_hybrid_analyzer.py (1)
171-192: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that the upstream exception text does not leak into the result.
The test uses the message
"secret upstream detail"to model sensitive upstream detail. It does not verify that the message is absent from the returned payload. Add an explicit assertion so a future change that surfacesstr(exception)fails this test.💚 Proposed addition
assert ( result["gemini"]["error_message"] == "GEMINI_ANALYZER_FAILED" ) + assert "secret upstream detail" not in str(result)🤖 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/analysis/text/test_hybrid_analyzer.py` around lines 171 - 192, Update test_uses_stacking_when_gemini_raises to assert that the returned result contains no occurrence of the sensitive text "secret upstream detail", especially within result["gemini"]["error_message"]. Preserve the existing fallback and sanitized error-code assertions.app/analysis/text/hybrid_analyzer.py (2)
106-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilence the BLE001 warnings for the intentional fail-safe catches.
Both broad catches are deliberate. They normalize engine failures instead of propagating them.
app/analysis/text/stacking_analyzer.pyalready marks the same pattern with# noqa: BLE001. Apply the same marker here for consistency with the linter configuration.♻️ Proposed change
- except Exception as exception: + except Exception as exception: # noqa: BLE001Also applies to: 172-172
🤖 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 `@app/analysis/text/hybrid_analyzer.py` at line 106, Add the # noqa: BLE001 marker to both intentional broad Exception handlers in the relevant analyzer methods, including the catches around the lines represented by “except Exception as exception.” Match the existing suppression pattern used in stacking_analyzer.py without changing the fail-safe behavior.Source: Linters/SAST tools
269-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
routing.decision.valueinstead of the hardcoded enum.This branch runs only when
stacking_analysis["is_available"]is falsy.ConditionalGeminiPolicy.routealready returnsGEMINI_FALLBACKfor that input, so the hardcoded value duplicates the policy decision. The adjacentrouting_reasonalready reads fromrouting. Reading both fields fromroutingkeeps the metadata consistent if the policy adds a new unavailable-model decision later.♻️ Proposed change
- "routing_decision": ( - HybridRoutingDecision - .GEMINI_FALLBACK - .value - ), + "routing_decision": ( + routing.decision.value + ),Remove the now-unused
HybridRoutingDecisionimport if no other reference remains.🤖 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 `@app/analysis/text/hybrid_analyzer.py` around lines 269 - 273, Update the unavailable-model branch in the hybrid analysis flow to set routing_decision from routing.decision.value, matching the adjacent routing_reason metadata and preserving the policy’s returned decision. Remove the HybridRoutingDecision import if no other references remain.tests/analysis/test_hybrid_policy.py (1)
82-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the two uncovered guard branches and the threshold boundaries.
ConditionalGeminiPolicy.routehas two fail-safe branches that no test exercises:STACKING_PROBABILITY_UNAVAILABLE(line 92 ofapp/analysis/hybrid_policy.py) andINVALID_STACKING_PROBABILITY(line 103). Both prevent a malformed stacking result from being treated as normal. The boundary valuesnormal_maxandphishing_minare also untested, and the comparisons use<=and>=.💚 Proposed additional tests
`@pytest.mark.parametrize`( "probability", [None, "0.5", float("nan"), 1.5, -0.1], ) def test_falls_back_to_gemini_for_unusable_probability( policy: ConditionalGeminiPolicy, probability, ) -> None: result = policy.route( { "engine": "stacking", "is_available": True, "result": {"risk_probability": probability}, } ) assert ( result.decision == HybridRoutingDecision.GEMINI_FALLBACK ) assert result.should_call_gemini is True `@pytest.mark.parametrize`( ("probability", "expected"), [ (0.2, HybridRoutingDecision.SELF_MODEL_NORMAL), (0.8, HybridRoutingDecision.SELF_MODEL_PHISHING), ], ) def test_threshold_boundaries_are_inclusive( policy: ConditionalGeminiPolicy, probability: float, expected: HybridRoutingDecision, ) -> None: result = policy.route( build_stacking_result(probability) ) assert result.decision == expected assert result.should_call_gemini is False🤖 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/analysis/test_hybrid_policy.py` around lines 82 - 119, Add tests in tests/analysis/test_hybrid_policy.py covering ConditionalGeminiPolicy.route’s unavailable and invalid stacking-probability fallback branches with None, nonnumeric, NaN, and out-of-range values, asserting GEMINI_FALLBACK and should_call_gemini=True. Also add boundary tests for HybridThresholds using probabilities equal to normal_max and phishing_min, asserting the inclusive SELF_MODEL_NORMAL and SELF_MODEL_PHISHING decisions without Gemini.tests/analysis/test_service.py (1)
120-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact final score for the Gemini escalation path.
assert result.final_score >= 40passes for a wide range of values. It does not distinguish the Gemini score of 90 from the blended text-track score thatRiskScoringEnginecurrently computes fromself_model_score=50andselected_score=90. See the related comment onapp/analysis/service.pyLines 331-336.Assert the exact expected score and the text contribution. A precise assertion documents which score the pipeline applies and fails if the blending semantics change.
💚 Proposed change
assert result.status == "SUCCESS" assert result.text_analysis["decision_source"] == "GEMINI" assert result.text_analysis["gemini_available"] is True - assert result.final_score >= 40 + # 텍스트 트랙이 어떤 점수를 적용했는지 명시적으로 고정 + assert result.contribution_breakdown.llm == <expected> + assert result.final_score == <expected>🤖 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/analysis/test_service.py` around lines 120 - 123, Update the Gemini escalation assertions in the test to verify the exact expected final_score produced by the current blending logic, and assert the corresponding text contribution as described by RiskScoringEngine. Replace the broad lower-bound check while preserving the existing status, decision_source, and gemini_available assertions.
🤖 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 `@app/analysis/service.py`:
- Around line 331-336: The service text-track call at app/analysis/service.py
lines 331-336 should pass only the HybridTextAnalyzer-selected score as the
single text signal, remove the raw naive_bayes_score=self_model_score argument,
and derive llm_available from whether a selected score exists. Update
tests/analysis/test_service.py lines 120-123 to assert the exact
result.final_score and result.contribution_breakdown.llm values, pinning the
applied selected text score.
- Around line 136-146: Update the asyncio.gather flow around text_task and
url_task to collect both task outcomes and handle exceptions explicitly. When
either task fails, cancel the sibling task, await its completion, and preserve
the existing fail-safe ERROR response; ensure both task results or exceptions
are retrieved so no background work or unhandled task warning remains.
In `@app/analysis/text/hybrid_analyzer.py`:
- Around line 104-105: Update the async analyze flow around
self.stacking_analyzer in HybridAnalyzer.analyze to execute the synchronous
callable via asyncio.to_thread, adding the asyncio import. Preserve the existing
arguments, result handling, and exception behavior while preventing blocking of
the event loop.
In `@app/core/config.py`:
- Around line 20-32: Update the default values for
STACKING_NORMAL_PROBABILITY_MAX and STACKING_PHISHING_PROBABILITY_MIN to
conservative, non-degenerate thresholds such as 0.1 and 0.9, while preserving
their existing 0–1 validation. Ensure the defaults no longer make nearly every
probability fall into the uncertain band.
In `@app/infrastructure/rabbitmq/result_factory.py`:
- Around line 163-165: Update the result mapping around selfModelConfidence and
add a _confidence helper next to _integer_score. Have the helper reject
booleans, non-numeric values, and NaN as None, while converting valid numbers to
float and clamping finite values to the 0.0–1.0 range before assigning
selfModelConfidence.
In `@data_science/SMSModel/artifacts/stacking/gemini_validation_predictions.json`:
- Around line 1-108: Document that the committed gemini_validation_predictions
cache is intentionally partial because of Gemini quota limits, including that
unavailable entries prevent threshold selection and are not a regression. Add
this note to the relevant PR or documentation while leaving the
fingerprint-and-score cache unchanged.
In `@data_science/SMSModel/run_hybrid_threshold_selection.py`:
- Around line 351-370: Update the score validation in the available calculation
to explicitly reject bool values while continuing to accept only integer scores
in the existing 0–100 range; preserve the current integer risk_score contract
and downstream _ordered_gemini_scores behavior.
In `@docs/PII_MASKING.md`:
- Around line 24-26: Update the deployment sequence in the SafeFam_BE/SafeFam_AI
token-format migration instructions to deploy SafeFam_AI before SafeFam_BE.
Alternatively, require both token formats to be supported by SafeFam_AI before
deploying the new-emitting SafeFam_BE version.
In `@docs/SCORING_PIPELINE_CHANGES.md`:
- Around line 109-120: Update docs/SCORING_PIPELINE_CHANGES.md lines 109-120 to
distinguish final-grade thresholds (0–39 LOW, 40–69 MEDIUM, 70–100 HIGH) from
the separate thresholds that route uncertain results to Gemini, documenting the
selected hybrid policy without conflating the two. Update docs/TRAINING_FLOW.md
lines 74-82 to include Recall, F2, Gemini call rate, fingerprint caching, policy
metadata, and the provisional validation state.
- Around line 123-136: Update the “테스트 현황” section to report the complete
validation result, including 434 passed and 3 deselected tests, the exact test
scope, and the Gemini quota limitation. Mark the final thresholds and Gemini
call rate as provisional until those limitations are resolved, while retaining
the existing list of relevant tests and out-of-scope files.
- Around line 5-17: Update the documentation to match the current
Stacking/Gemini contract: in docs/SCORING_PIPELINE_CHANGES.md lines 5-17,
replace the Naive Bayes-first and SAFE-only routing diagram with high-confidence
and uncertain-result routing; in docs/PII_MASKING.md lines 18-20, identify the
Stacking hybrid analyzer and Gemini uncertainty path; and in
docs/TRAINING_FLOW.md line 172, replace Claude and medium-only escalation
terminology with Gemini and the current routing policy.
- Around line 102-105: Update the scoring documentation around the VirusTotal
formula to state that raw_score is normalized to 0–1, while
app/infrastructure/rabbitmq/result_factory.py:_url_score converts it to 0–100
for external output. Add boundary examples for raw scores 0 and 1.0, plus the
zero-engines case, and clarify that callers must not apply the conversion twice.
In `@docs/TRAINING_FLOW.md`:
- Around line 113-118: The documentation for generate_voice_data.py must clarify
that its precomputed 68/16/16 split assignment is informational because
_leak_free_split() reassigns splits; alternatively remove the precomputed split
claim. Explicitly state whether each 15% allocation is calculated from the
original dataset or from the remaining subset, consistently in both affected
sections.
---
Nitpick comments:
In `@app/analysis/text/hybrid_analyzer.py`:
- Line 106: Add the # noqa: BLE001 marker to both intentional broad Exception
handlers in the relevant analyzer methods, including the catches around the
lines represented by “except Exception as exception.” Match the existing
suppression pattern used in stacking_analyzer.py without changing the fail-safe
behavior.
- Around line 269-273: Update the unavailable-model branch in the hybrid
analysis flow to set routing_decision from routing.decision.value, matching the
adjacent routing_reason metadata and preserving the policy’s returned decision.
Remove the HybridRoutingDecision import if no other references remain.
In `@data_science/SMSModel/modeling/hybrid_thresholds.py`:
- Around line 185-236: Reduce the threshold-search cost in the candidate
generation and nested loops by bounding observed probability candidates to a
fixed-size quantile grid (for example, at most 200 evenly spaced quantiles)
while retaining the fixed 0.0–1.0 boundaries and uniqueness. Preserve valid
normal_max/phishing_min ordering and the existing metric-based selection
behavior.
In `@data_science/SMSModel/run_hybrid_threshold_selection.py`:
- Around line 101-176: The checksum validation in _load_stacking_classifier only
detects corruption or stale artifacts, not tampering, because model_sha256 is
read from the colocated metadata.json and joblib.load can execute pickle code.
Add a short comment immediately before the checksum comparison or
deserialization documenting this trust boundary and explicitly stating that the
check is not a security control.
- Around line 441-464: Define a single module-level constant for the Gemini
phishing score, then replace the hardcoded 40 in _write_policy_report and the
other usage near line 527, and use the same constant as the default
gemini_phishing_score in select_hybrid_thresholds so selection and reporting
remain consistent.
In `@tests/analysis/test_execution.py`:
- Around line 35-56: Extend the _text_analysis test-data builder to include
decision_source, routing_reason, and fallback_applied in the result payload,
plus confidence under self_model, using the shared hybrid response values and
defaults established by the RabbitMQ fixtures. Preserve the existing score,
error, and Gemini-related fields while keeping the returned schema consistent
for classifier tests.
In `@tests/analysis/test_hybrid_policy.py`:
- Around line 82-119: Add tests in tests/analysis/test_hybrid_policy.py covering
ConditionalGeminiPolicy.route’s unavailable and invalid stacking-probability
fallback branches with None, nonnumeric, NaN, and out-of-range values, asserting
GEMINI_FALLBACK and should_call_gemini=True. Also add boundary tests for
HybridThresholds using probabilities equal to normal_max and phishing_min,
asserting the inclusive SELF_MODEL_NORMAL and SELF_MODEL_PHISHING decisions
without Gemini.
In `@tests/analysis/test_service.py`:
- Around line 120-123: Update the Gemini escalation assertions in the test to
verify the exact expected final_score produced by the current blending logic,
and assert the corresponding text contribution as described by
RiskScoringEngine. Replace the broad lower-bound check while preserving the
existing status, decision_source, and gemini_available assertions.
In `@tests/analysis/text/test_hybrid_analyzer.py`:
- Around line 171-192: Update test_uses_stacking_when_gemini_raises to assert
that the returned result contains no occurrence of the sensitive text "secret
upstream detail", especially within result["gemini"]["error_message"]. Preserve
the existing fallback and sanitized error-code assertions.
In `@tests/data_science/SMSModel/modeling/test_hybrid_thresholds.py`:
- Around line 105-152: Add parametrized coverage in test_rejects_invalid_inputs
for a single-label labels array to exercise the label-set validation, and add a
separate test for select_hybrid_thresholds where Gemini and stacking both
misclassify so no threshold pair reaches target_recall and the
unreachable-target RuntimeError is asserted. Use the existing test helpers and
invocation style.
In `@tests/infrastructure/rabbitmq/test_result_factory.py`:
- Around line 144-153: Add test cases alongside the existing STACKING_GEMINI
assertions to cover metadata mapping for a fallback result with
fallback_applied=True, expecting TextAnalysisMethod.STACKING_FALLBACK, and an
all-engines-unavailable result, expecting TextAnalysisMethod.UNAVAILABLE and the
corresponding outcome metadata. Reuse the existing event factory and assertion
style so downstream-readable fields are verified for both paths.
🪄 Autofix
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: 5bba9692-de3c-4670-bef0-161d9095a7e9
📒 Files selected for processing (22)
app/analysis/execution.pyapp/analysis/hybrid_policy.pyapp/analysis/service.pyapp/analysis/text/hybrid_analyzer.pyapp/core/config.pyapp/infrastructure/rabbitmq/result_factory.pyapp/infrastructure/rabbitmq/schemas.pydata_science/SMSModel/artifacts/stacking/gemini_validation_predictions.jsondata_science/SMSModel/modeling/hybrid_thresholds.pydata_science/SMSModel/run_hybrid_threshold_selection.pydocs/PII_MASKING.mddocs/SCORING_PIPELINE_CHANGES.mddocs/TRAINING_FLOW.mdtests/analysis/test_execution.pytests/analysis/test_hybrid_policy.pytests/analysis/test_router.pytests/analysis/test_service.pytests/analysis/text/test_hybrid_analyzer.pytests/core/test_config.pytests/data_science/SMSModel/modeling/test_hybrid_thresholds.pytests/infrastructure/rabbitmq/test_consumer.pytests/infrastructure/rabbitmq/test_result_factory.py
🛑 Comments failed to post (6)
docs/PII_MASKING.md (1)
24-26: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Deploy the consumer before the producer after token-format changes.
If
SafeFam_BEemits a new token format beforeSafeFam_AIsupports it, FastAPI can miss token-specific preprocessing and rules. DeploySafeFam_AIfirst, or support both token formats before deployingSafeFam_BE.Proposed documentation change
-3. Spring → FastAPI 순서로 배포 +3. FastAPI(SafeFam_AI) → Spring(SafeFam_BE) 순서로 배포📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.1. `SafeFam_BE`와 `SafeFam_AI` 양쪽 레포에서 토큰 형식 동시 수정 2. 양쪽 테스트 통과 확인 3. FastAPI(SafeFam_AI) → Spring(SafeFam_BE) 순서로 배포🤖 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 `@docs/PII_MASKING.md` around lines 24 - 26, Update the deployment sequence in the SafeFam_BE/SafeFam_AI token-format migration instructions to deploy SafeFam_AI before SafeFam_BE. Alternatively, require both token formats to be supported by SafeFam_AI before deploying the new-emitting SafeFam_BE version.docs/SCORING_PIPELINE_CHANGES.md (4)
5-17: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the documentation with the current Stacking/Gemini contract.
docs/SCORING_PIPELINE_CHANGES.md#L5-L17: Replace the Naive Bayes-first diagram and SAFE-only bypass description with the current high-confidence and uncertain-result routing.docs/PII_MASKING.md#L18-L20: Name the current Stacking hybrid analyzer and Gemini uncertainty route.docs/TRAINING_FLOW.md#L172: Replace Claude terminology and medium-only escalation with Gemini and the current routing policy.🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 3 files
docs/SCORING_PIPELINE_CHANGES.md#L5-L17(this comment)docs/PII_MASKING.md#L18-L20docs/TRAINING_FLOW.md#L172-L172🤖 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 `@docs/SCORING_PIPELINE_CHANGES.md` around lines 5 - 17, Update the documentation to match the current Stacking/Gemini contract: in docs/SCORING_PIPELINE_CHANGES.md lines 5-17, replace the Naive Bayes-first and SAFE-only routing diagram with high-confidence and uncertain-result routing; in docs/PII_MASKING.md lines 18-20, identify the Stacking hybrid analyzer and Gemini uncertainty path; and in docs/TRAINING_FLOW.md line 172, replace Claude and medium-only escalation terminology with Gemini and the current routing policy.
102-105: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C 5 'malicious|suspicious|ratio|score' \ app/service/security/virustotal.py \ app/service/security/hybrid_url_engine.py \ app/utils/scoring_engine.pyRepository: SafeFam/SafeFam_AI
Length of output: 393
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- candidate files ---' git ls-files | rg -i '(^|/)(virustotal|hybrid.*url|scoring.*engine|score|pipeline).*|SCORING_PIPELINE_CHANGES\.md$' || true printf '%s\n' '--- relevant symbols ---' rg -n -i -C 3 'malicious|suspicious|전체엔진수|VirusTotal|virustotal|hybrid_url_engine|ratio' . \ -g '!node_modules' -g '!dist' -g '!build' \ | head -n 400Repository: SafeFam/SafeFam_AI
Length of output: 26177
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- raw score consumers ---' rg -n -C 5 '\braw_score\b|\burl_risk_score\b|\bscore\b' app tests docs \ -g '*.py' -g '*.md' | head -n 500 printf '%s\n' '--- implementation structure ---' ast-grep outline app/infrastructure/virustotal/client.py fd -i -t f 'scoring|hybrid|url' app tests docs | head -n 200 printf '%s\n' '--- focused source ---' cat -n app/infrastructure/virustotal/client.py | sed -n '100,142p' printf '%s\n' '--- documentation ---' cat -n docs/SCORING_PIPELINE_CHANGES.md | sed -n '95,145p'Repository: SafeFam/SafeFam_AI
Length of output: 37818
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- URL analyzer scoring path ---' cat -n app/analysis/url/analyzer.py | sed -n '120,185p' printf '%s\n' '--- public score conversion ---' rg -n -C 8 'def _url_score|def _integer_score|final_score|weighted_contributions|url_risk_score' \ app/infrastructure/rabbitmq/result_factory.py app/analysis/scoring.py app/analysis/hybrid_policy.py printf '%s\n' '--- focused tests ---' cat -n tests/infrastructure/test_virustotal_client.py | sed -n '1,145p' rg -n -C 5 'url_risk_score|_url_score|0\.0|100|boundary|score' tests app/infrastructure/rabbitmq \ -g '*.py' | head -n 400 printf '%s\n' '--- exact formula and conversion probe ---' python3 - <<'PY' from decimal import Decimal, ROUND_HALF_UP def vt_raw(malicious, suspicious, total): if total <= 0: return 0.0 return round(min(malicious / total + (suspicious / total) * 0.5, 1.0), 2) def public_score(value): # Candidate behavior to compare with the repository's eventual _url_score implementation. return round(value * 100) for case in [(0, 0, 0), (1, 0, 90), (0, 1, 90), (10, 0, 100), (0, 10, 100), (100, 0, 100), (90, 20, 110)]: raw = vt_raw(*case) print(case, 'raw=', raw, 'public_if_scaled=', public_score(raw)) PYRepository: SafeFam/SafeFam_AI
Length of output: 48805
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- URL analyzer scoring path ---' cat -n app/analysis/url/analyzer.py | sed -n '120,185p' printf '%s\n' '--- public score conversion ---' rg -n -C 8 'def _url_score|def _integer_score|final_score|weighted_contributions|url_risk_score' \ app/infrastructure/rabbitmq/result_factory.py app/analysis/scoring.py app/analysis/hybrid_policy.py printf '%s\n' '--- focused tests ---' cat -n tests/infrastructure/test_virustotal_client.py | sed -n '1,145p' rg -n -C 5 'url_risk_score|_url_score|0\.0|100|boundary|score' tests app/infrastructure/rabbitmq \ -g '*.py' | head -n 400 printf '%s\n' '--- exact formula and conversion probe ---' python3 - <<'PY' def vt_raw(malicious, suspicious, total): if total <= 0: return 0.0 return round(min(malicious / total + (suspicious / total) * 0.5, 1.0), 2) def public_score(value): return round(value * 100) for case in [(0, 0, 0), (1, 0, 90), (0, 1, 90), (10, 0, 100), (0, 10, 100), (100, 0, 100), (90, 20, 110)]: raw = vt_raw(*case) print(case, 'raw=', raw, 'public_if_scaled=', public_score(raw)) PYRepository: SafeFam/SafeFam_AI
Length of output: 49111
Document the VirusTotal score units.
The formula produces a normalized
raw_scorein0–1. State thatapp/infrastructure/rabbitmq/result_factory.py:_url_scoreconverts it to0–100for external output. Add boundary examples for0,1.0, and zero engines to prevent double conversion.🤖 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 `@docs/SCORING_PIPELINE_CHANGES.md` around lines 102 - 105, Update the scoring documentation around the VirusTotal formula to state that raw_score is normalized to 0–1, while app/infrastructure/rabbitmq/result_factory.py:_url_score converts it to 0–100 for external output. Add boundary examples for raw scores 0 and 1.0, plus the zero-engines case, and clarify that callers must not apply the conversion twice.
109-120: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Document the selected hybrid policy separately from model scoring.
docs/SCORING_PIPELINE_CHANGES.md#L109-L120: Identify which thresholds classify the final grade and which thresholds route uncertain results to Gemini.docs/TRAINING_FLOW.md#L74-L82: Add Recall, F2, Gemini call rate, fingerprint caching, policy metadata, and the provisional validation state.🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 111-111: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 2 files
docs/SCORING_PIPELINE_CHANGES.md#L109-L120(this comment)docs/TRAINING_FLOW.md#L74-L82🤖 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 `@docs/SCORING_PIPELINE_CHANGES.md` around lines 109 - 120, Update docs/SCORING_PIPELINE_CHANGES.md lines 109-120 to distinguish final-grade thresholds (0–39 LOW, 40–69 MEDIUM, 70–100 HIGH) from the separate thresholds that route uncertain results to Gemini, documenting the selected hybrid policy without conflating the two. Update docs/TRAINING_FLOW.md lines 74-82 to include Recall, F2, Gemini call rate, fingerprint caching, policy metadata, and the provisional validation state.
123-136: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Publish the complete and current validation status.
This section reports 59 passing tests but omits the broader
434 passed, 3 deselectedresult and the Gemini quota limitation. The final thresholds and Gemini call rate remain undetermined. Document the exact test scope and mark the policy values as provisional.🤖 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 `@docs/SCORING_PIPELINE_CHANGES.md` around lines 123 - 136, Update the “테스트 현황” section to report the complete validation result, including 434 passed and 3 deselected tests, the exact test scope, and the Gemini quota limitation. Mark the final thresholds and Gemini call rate as provisional until those limitations are resolved, while retaining the existing list of relevant tests and out-of-scope files.docs/TRAINING_FLOW.md (1)
113-118: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve the Voice split contract.
generate_voice_data.pyis documented as assigning68/16/16split metadata, but_leak_free_split()is documented as ignoring that metadata and re-splitting the data. State that the generated split is informational, or remove it. Also specify whether each15%value is calculated from the original dataset or the remaining subset.Also applies to: 129-135
🤖 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 `@docs/TRAINING_FLOW.md` around lines 113 - 118, The documentation for generate_voice_data.py must clarify that its precomputed 68/16/16 split assignment is informational because _leak_free_split() reassigns splits; alternatively remove the precomputed split claim. Explicitly state whether each 15% allocation is calculated from the original dataset or from the remaining subset, consistently in both affected sections.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@app/analysis/text/hybrid_analyzer.py`:
- Around line 133-140: Update the force-gemini override in the hybrid analyzer
to replace the routing result whenever force_gemini is True, regardless of
routing.should_call_gemini, preserving decision GEMINI_REVIEW,
should_call_gemini=True, and reason RULE_RISK_ESCALATION. Add coverage for an
uncertain Stacking score with force_gemini=True.
🪄 Autofix
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: 6c8a9d9f-0a42-444e-a880-a8e2587ac6d4
📒 Files selected for processing (4)
app/analysis/service.pyapp/analysis/text/hybrid_analyzer.pytests/analysis/test_service.pytests/analysis/text/test_hybrid_analyzer.py
pearseona
changed the title
1 participant
📝 개요
PR #52에서 구현한 Stacking 자체 모델의 위험 확률을 기반으로, 자체 모델의 예측이 불확실한 메시지만 Gemini에 재검증을 요청하는 하이브리드 텍스트 분석 파이프라인을 구현했습니다.
Stacking 위험 확률이 확신 구간에 포함되면 자체 모델 결과를 즉시 사용하고, 두 임계값 사이의 불확실 구간에서는 Gemini 분석 결과를 최종 판정으로 사용합니다.
Gemini 호출 실패, timeout 또는 응답 파싱 실패 시에는 Stacking 결과로 fallback하며, 두 분석 엔진이 모두 실패한 경우
UNKNOWN으로 처리해 정상 메시지로 오판하는 fail-open을 방지했습니다.🔗 관련 이슈
🎯 주요 변경 사항
Gemini 조건부 호출 정책 구현
risk_probability를 기준으로 정상·불확실·위험 구간 분류하이브리드 최종 판정 구현
장애 fallback 구현
UNKNOWN과ALL_TEXT_ENGINES_UNAVAILABLE반환임계값 선정 도구 구현
분석 응답 및 RabbitMQ 연동
TEXT:STACKING,TEXT:GEMINI,TEXT실패 상태 분류COMPLETED,PARTIAL,FAILED결과 이벤트 반영📊 검증 결과
1.00000.9153123건17건13건미확정미확정미확정미확정미확정434 passed, 3 deselected✅ PR 체크리스트
uvicorn구동 또는 테스트 코드)를 통과했습니다.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests