Feat(#36): 형태소·문자 기반 피싱 분류 모델 추가 및 성능 비교 (1/2) - #38
Merged
Conversation
|
Warning Review limit reached
Next review available in: 44 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 (6)
📝 WalkthroughWalkthroughThis PR adds shared SMS preprocessing, Kiwi tokenization, leakage-safe grouped splits, Naive Bayes evaluation, reporting, versioned artifact export, and updated model paths. It also reformats unrelated application and test files without changing their behavior. ChangesSMS model pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant TrainingPipeline
participant TemplateGrouping
participant DatasetSplitter
participant Evaluator
participant ArtifactStore
participant SMSAnalyzer
TrainingPipeline->>TemplateGrouping: fingerprint, deduplicate, and group SMS messages
TemplateGrouping->>DatasetSplitter: provide grouped dataset
DatasetSplitter-->>TrainingPipeline: return validated train, validation, and test splits
TrainingPipeline->>Evaluator: train models and select validation thresholds
Evaluator->>ArtifactStore: save versioned model and vectorizer artifacts
SMSAnalyzer->>ArtifactStore: resolve current.json and load active artifacts
SMSAnalyzer-->>TrainingPipeline: return phishing probability and risk result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 12
🧹 Nitpick comments (2)
data_science/SMSModel/train_sms.py (2)
235-324: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider failing when the committed manifest is absent.
The README states that the committed manifest fixes the final test set. When
SPLIT_MANIFEST_PATHdoes not exist andcreate_manifestisFalse,split_datagenerates and saves a new manifest anyway.run_naive_bayes_baseline.pycallssplit_data(dataset)with the same defaults, so baseline metrics can be produced against an ad-hoc test set. The print at Line 274 is the only signal. Requirecreate_manifest=Trueto create a manifest, so an accidental regeneration stops the run.🤖 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/train_sms.py` around lines 235 - 324, Update split_data so that when SPLIT_MANIFEST_PATH is absent and create_manifest is False, it raises an error instead of generating or saving a new split; only the explicit create_manifest=True path may call split_grouped_dataset and save_split_manifest, while the existing manifest-loading path remains unchanged.
141-142: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe null check covers columns that the schema does not require.
required_columnslists onlytext,label,type, andhas_url.df.isnull().any().any()rejects nulls in every column, including the optionalsourcecolumn that Line 160 reads with a default. A dataset that adds any optional column with blank cells fails to load, and the error message does not name the column. Restrict the check to the required columns and report the offending ones.♻️ Proposed change
- if df.isnull().any().any(): - raise ValueError("결측치가 존재합니다.") + null_columns = [ + column + for column in required_columns + if df[column].isnull().any() + ] + if null_columns: + raise ValueError(f"결측치가 존재하는 컬럼: {sorted(null_columns)}")🤖 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/train_sms.py` around lines 141 - 142, Update the null validation in the training-data loading flow to inspect only the columns listed by required_columns (text, label, type, and has_url), leaving optional columns such as source eligible for the existing default handling. Collect the required columns containing nulls and include their names in the ValueError message instead of reporting a generic failure.
🤖 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/naive_bayes_analyzer.py`:
- Around line 103-110: Update the result construction in the analyzer method so
result["error_message"] always uses DEFAULT_ANALYSIS_RESULT["error_message"]
rather than _load_error; retain _load_error only for internal logging where the
exception type belongs.
- Around line 58-61: Update _load_artifacts to synchronize concurrent callers
with a lock: acquire the lock before checking _load_attempted, re-check the flag
after acquiring it, and keep the flag unset until the artifact load completes or
otherwise ensure waiting callers do not return while _model is still
unavailable. Preserve the existing one-time loading behavior and have concurrent
startup requests observe the successfully loaded model.
- Around line 116-119: The single-message feature construction in
naive_bayes_analyzer.py and predict_risk_score does not reproduce the declared
has_url training signal. Update each extract_struct_features call to pass the
same has_url value used by extract_struct_feature_matrix, including the site at
data_science/SMSModel/train_sms.py:546, or consistently derive that value
identically during training and serving.
In `@data_science/SMSModel/dataset_splitting/manifest.py`:
- Around line 14-20: Update the manifest-building and saving flow to accept and
use DatasetSplitConfig, including build_split_manifest and save_split_manifest,
instead of relying on fixed MANIFEST_COLUMNS names. Derive the fingerprint,
group, and label columns from the configuration so renamed key columns work end
to end, and add a round-trip test covering custom column names.
In `@data_science/SMSModel/dataset_splitting/splitter.py`:
- Around line 85-92: Update _select_best_group_split and its
candidate-generation flow so candidates vary across feasible group counts
instead of fixing one count before scoring. Ensure _candidate_score evaluates
each whole-group split against the configured train, validation, and test row
ratios (while preserving label-distribution scoring), allowing unequal group
sizes to be optimized for row-level targets.
In `@data_science/SMSModel/evaluation/threshold.py`:
- Around line 53-58: Update the validation logic in the threshold-selection
function around allowed_labels to reject y_true when it contains only one
distinct supported label, raising ValueError before computing or returning any
score-derived threshold. Preserve the existing unsupported-label validation, and
add coverage for normal-only and phishing-only validation data.
In `@data_science/SMSModel/modeling/artifacts.py`:
- Around line 51-65: The save_operational_naive_bayes_artifacts() flow currently
exposes model_path and vectorizer_path independently, allowing mixed artifact
generations during reloads. Store both outputs in a new versioned directory,
then atomically update a single manifest or pointer only after both files are
successfully written; update the API loader to resolve both artifacts through
that shared pointer.
In `@data_science/SMSModel/modeling/base.py`:
- Around line 28-37: Update __post_init__ to assign the normalized result of
np.asarray(self.values) back to self.values before validation, so list inputs
are stored as NumPy arrays. Preserve the existing dimensionality, finiteness,
and probability-range checks.
In `@data_science/SMSModel/modeling/naive_bayes.py`:
- Around line 210-228: Before constructing or fitting CalibratedClassifierCV in
the training flow, use train_df["label"].value_counts().min() to validate that
every class has at least self.calibration_cv samples. Raise the established
validation error when the minimum count is insufficient, while preserving
_validate_dataframe and the existing calibration setup for valid datasets.
In `@data_science/SMSModel/README.md`:
- Around line 3-13: Add tokenization/, modeling/, evaluation/, and
run_naive_bayes_baseline.py as rows in the workspace table, with concise
purposes matching their roles, so the README inventory includes every newly
added package and baseline runner.
In `@Dockerfile`:
- Around line 13-19: Ensure the runtime image includes the module path required
by the serialized vectorizer’s kiwi_tokenize callable. Update the Dockerfile
COPY steps to include data_science/SMSModel/tokenization/__init__.py and
kiwi_tokenizer.py, or relocate kiwi_tokenize under an already-copied package
while preserving its import path at model load time.
In `@tests/data_science/SMSModel/test_template_grouping.py`:
- Around line 305-307: Rename the unused holdout result binding in the load_data
call to _df_holdout, while preserving df_pool and the existing load_data
invocation.
---
Nitpick comments:
In `@data_science/SMSModel/train_sms.py`:
- Around line 235-324: Update split_data so that when SPLIT_MANIFEST_PATH is
absent and create_manifest is False, it raises an error instead of generating or
saving a new split; only the explicit create_manifest=True path may call
split_grouped_dataset and save_split_manifest, while the existing
manifest-loading path remains unchanged.
- Around line 141-142: Update the null validation in the training-data loading
flow to inspect only the columns listed by required_columns (text, label, type,
and has_url), leaving optional columns such as source eligible for the existing
default handling. Collect the required columns containing nulls and include
their names in the ValueError message instead of reporting a generic failure.
🪄 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: 870ca283-5df5-497c-8d82-5dfa1d9579aa
⛔ Files ignored due to path filters (8)
data_science/SMSModel/artifacts/phishing_model_artifact.pklis excluded by!**/*.pkldata_science/SMSModel/artifacts/phishing_vectorizer.pklis excluded by!**/*.pkldata_science/SMSModel/reports/feature_scores_full.csvis excluded by!**/*.csvdata_science/SMSModel/reports/figures/feature_importance.pngis excluded by!**/*.pngdata_science/SMSModel/reports/figures/risk_distribution_fig1.pngis excluded by!**/*.pngdata_science/SMSModel/reports/figures/risk_distribution_fig2.pngis excluded by!**/*.pngdata_science/SMSModel/reports/model_evaluation/naive_bayes_baseline/model_evaluation.csvis excluded by!**/*.csvdata_science/SMSModel/splits/sms_split_v1.csvis excluded by!**/*.csv
📒 Files selected for processing (62)
.dockerignore.env.exampleDockerfileSCORING_PIPELINE_CHANGES.mdapp/analysis/text/naive_bayes_analyzer.pyapp/analysis/text/preprocessing.pyapp/core/config.pydata_science/SMSModel/README.mddata_science/SMSModel/SMSDataModel.ipynbdata_science/SMSModel/dataset_splitting/__init__.pydata_science/SMSModel/dataset_splitting/config.pydata_science/SMSModel/dataset_splitting/manifest.pydata_science/SMSModel/dataset_splitting/splitter.pydata_science/SMSModel/dataset_splitting/validation.pydata_science/SMSModel/evaluation/__init__.pydata_science/SMSModel/evaluation/evaluator.pydata_science/SMSModel/evaluation/latency.pydata_science/SMSModel/evaluation/metrics.pydata_science/SMSModel/evaluation/reporting.pydata_science/SMSModel/evaluation/threshold.pydata_science/SMSModel/modeling/__init__.pydata_science/SMSModel/modeling/artifacts.pydata_science/SMSModel/modeling/base.pydata_science/SMSModel/modeling/naive_bayes.pydata_science/SMSModel/reporting/__init__.pydata_science/SMSModel/reporting/dataset_split_report.pydata_science/SMSModel/reports/dataset_split_summary.jsondata_science/SMSModel/reports/dataset_split_summary.mddata_science/SMSModel/reports/model_evaluation/naive_bayes_baseline/model_evaluation.jsondata_science/SMSModel/reports/model_evaluation/naive_bayes_baseline/model_evaluation.mddata_science/SMSModel/run_naive_bayes_baseline.pydata_science/SMSModel/template_grouping/__init__.pydata_science/SMSModel/template_grouping/config.pydata_science/SMSModel/template_grouping/fingerprint.pydata_science/SMSModel/template_grouping/service.pydata_science/SMSModel/template_grouping/similarity.pydata_science/SMSModel/tokenization/__init__.pydata_science/SMSModel/tokenization/kiwi_tokenizer.pydata_science/SMSModel/train_sms.pypytest.inirequirements.txttests/analysis/text/test_naive_bayes_analyzer.pytests/analysis/text/test_preprocessing.pytests/data_science/SMSModel/__init__.pytests/data_science/SMSModel/evaluation/__init__.pytests/data_science/SMSModel/evaluation/conftest.pytests/data_science/SMSModel/evaluation/test_base.pytests/data_science/SMSModel/evaluation/test_evaluator.pytests/data_science/SMSModel/evaluation/test_latency.pytests/data_science/SMSModel/evaluation/test_metrics.pytests/data_science/SMSModel/evaluation/test_reporting.pytests/data_science/SMSModel/evaluation/test_threshold.pytests/data_science/SMSModel/modeling/conftest.pytests/data_science/SMSModel/modeling/test_artifacts.pytests/data_science/SMSModel/modeling/test_baseline_runner.pytests/data_science/SMSModel/modeling/test_naive_bayes.pytests/data_science/SMSModel/test_dataset_split_report.pytests/data_science/SMSModel/test_dataset_splitting.pytests/data_science/SMSModel/test_template_grouping.pytests/data_science/SMSModel/tokenization/__init__.pytests/data_science/SMSModel/tokenization/test_kiwi_tokenizer.pytests/data_science/__init__.py
Comment on lines
+14
to
+20
| MANIFEST_COLUMNS = [ | ||
| "text_fingerprint", | ||
| "template_group_id", | ||
| "split", | ||
| "label", | ||
| "type", | ||
| ] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use DatasetSplitConfig when creating manifests.
DatasetSplitConfig supports custom key column names, but MANIFEST_COLUMNS always requires the default names. A non-default configuration can create valid splits, then fail in save_split_manifest with missing default columns.
Pass DatasetSplitConfig to build_split_manifest and save_split_manifest. Derive fingerprint, group, and label manifest columns from that config. Add a round-trip test with renamed columns.
Also applies to: 24-55
🤖 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/dataset_splitting/manifest.py` around lines 14 - 20,
Update the manifest-building and saving flow to accept and use
DatasetSplitConfig, including build_split_manifest and save_split_manifest,
instead of relying on fixed MANIFEST_COLUMNS names. Derive the fingerprint,
group, and label columns from the configuration so renamed key columns work end
to end, and add a round-trip test covering custom column names.
Comment on lines
+28
to
+37
| def __post_init__(self) -> None: | ||
| values = np.asarray(self.values) | ||
| if values.ndim != 1: | ||
| raise ValueError("score values must be one-dimensional") | ||
| if not np.isfinite(values).all(): | ||
| raise ValueError("score values must contain only finite numbers") | ||
| if self.score_type == ScoreType.PROBABILITY and ( | ||
| (values < 0.0) | (values > 1.0) | ||
| ).any(): | ||
| raise ValueError("probability scores must be between 0 and 1") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Store the normalized NumPy array in values.
Line 29 validates np.asarray(self.values) but leaves self.values unchanged. A list input passes validation and later exposes a list instead of the required np.ndarray.
Proposed fix
def __post_init__(self) -> None:
- values = np.asarray(self.values)
+ values = np.asarray(self.values, dtype=float)
if values.ndim != 1:
raise ValueError("score values must be one-dimensional")
if not np.isfinite(values).all():
raise ValueError("score values must contain only finite numbers")
+ object.__setattr__(self, "values", values)📝 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.
Suggested change
| def __post_init__(self) -> None: | |
| values = np.asarray(self.values) | |
| if values.ndim != 1: | |
| raise ValueError("score values must be one-dimensional") | |
| if not np.isfinite(values).all(): | |
| raise ValueError("score values must contain only finite numbers") | |
| if self.score_type == ScoreType.PROBABILITY and ( | |
| (values < 0.0) | (values > 1.0) | |
| ).any(): | |
| raise ValueError("probability scores must be between 0 and 1") | |
| def __post_init__(self) -> None: | |
| values = np.asarray(self.values, dtype=float) | |
| if values.ndim != 1: | |
| raise ValueError("score values must be one-dimensional") | |
| if not np.isfinite(values).all(): | |
| raise ValueError("score values must contain only finite numbers") | |
| object.__setattr__(self, "values", values) | |
| if self.score_type == ScoreType.PROBABILITY and ( | |
| (values < 0.0) | (values > 1.0) | |
| ).any(): | |
| raise ValueError("probability scores must be between 0 and 1") |
🤖 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/base.py` around lines 28 - 37, Update
__post_init__ to assign the normalized result of np.asarray(self.values) back to
self.values before validation, so list inputs are stored as NumPy arrays.
Preserve the existing dimensionality, finiteness, and probability-range checks.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
app/analysis/router.py (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilence Ruff B008 for FastAPI
Depends.
Depends(get_analysis_service)is valid FastAPI dependencies syntax. Add a project-level Ruff ignore orextend-immutable-callsexemption for this rule so CI lints do not block this valid code.🤖 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/router.py` at line 25, Update the project Ruff configuration to exempt FastAPI dependency calls such as Depends(get_analysis_service) from B008, using the appropriate ignore or extend-immutable-calls setting. Keep the dependency declaration in the router unchanged and apply the exemption project-wide.Source: Linters/SAST tools
tests/analysis/test_service.py (1)
145-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a discard name for the unused test value.
Ruff RUF059 flags
text_analysisin this unpacking. The test only checksnaive_bayes_scoreandllm_available. Rename the binding to_text_analysis, or assert its contents.♻️ Proposed fix
- text_analysis, + _text_analysis,🤖 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 145 - 149, Update the unpacking result from service._analyze_text_hybrid in the affected test to bind the unused first value as _text_analysis, while preserving the existing assertions for naive_bayes_score and llm_available.Source: Linters/SAST tools
🤖 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/scoring.py`:
- Around line 152-154: Update the scoring flow around _combine_text_track_score
so BOTH_ENGINES_UNAVAILABLE_FALLBACK_SCORE remains an effective text-track
contribution even when text_available is false. Ensure weight normalization and
llm_contrib use that fallback instead of assigning all weight to the zero-score
rules track; alternatively return the pipeline error response. Update the
regression scenario in tests/analysis/test_service.py to require at least
MEDIUM.
In `@data_science/SMSModel/README.md`:
- Line 21: Change the `실행 방법` heading in the README from its current level to an
H2 heading, preserving the existing heading text and surrounding content.
In `@data_science/SMSModel/SMSDataModel.ipynb`:
- Around line 139-140: Update the evaluation feature construction around
ts._extract_struct_features so it uses the same inputs as train_sms.py and the
API inference pipeline. Remove df_eval["has_url"] from this call unless URL
values are also propagated consistently through training, holdout evaluation,
and inference; keep the resulting X construction aligned with the trained
feature matrix.
In `@data_science/SMSModel/train_sms.py`:
- Around line 400-414: Update the model-selection condition around best.update
so it explicitly tracks whether the current candidate and best candidate satisfy
TARGET_PHISHING_RECALL. Prefer a target-satisfying candidate according to the
existing recall_normal ranking, allow later fallback candidates to replace
earlier fallback candidates when rec_p is higher, and never let a fallback
candidate displace a target-satisfying best candidate.
In `@data_science/VoiceModel/train_voice.py`:
- Around line 526-541: Update the model-selection condition in the training loop
so candidates meeting TARGET_PHISHING_RECALL always take precedence over
fallback candidates, regardless of recall_normal. Only use the best["model"] is
None fallback when no target-recall candidate has been selected, while
preserving the existing metric updates in best.update.
In `@data_science/VoiceModel/VoiceDataModel.ipynb`:
- Around line 167-182: Rebuild and save matching ComplementNB model and
vectorizer artifacts, then rerun the notebook so the scoring cell produces valid
risk_score values. In the scoring flow before model.predict_proba(X), add a
feature-count compatibility check between X and the model’s expected input
features, raising a direct message instructing users to regenerate matching
artifacts when they differ.
---
Nitpick comments:
In `@app/analysis/router.py`:
- Line 25: Update the project Ruff configuration to exempt FastAPI dependency
calls such as Depends(get_analysis_service) from B008, using the appropriate
ignore or extend-immutable-calls setting. Keep the dependency declaration in the
router unchanged and apply the exemption project-wide.
In `@tests/analysis/test_service.py`:
- Around line 145-149: Update the unpacking result from
service._analyze_text_hybrid in the affected test to bind the unused first value
as _text_analysis, while preserving the existing assertions for
naive_bayes_score and llm_available.
🪄 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: 0ce19419-efd5-47e7-a37a-0f0b5a3df3e6
📒 Files selected for processing (93)
Dockerfileapp/analysis/execution.pyapp/analysis/ports.pyapp/analysis/router.pyapp/analysis/rules/analyzer.pyapp/analysis/schemas.pyapp/analysis/scoring.pyapp/analysis/service.pyapp/analysis/text/gemini_analyzer.pyapp/analysis/text/naive_bayes_analyzer.pyapp/analysis/text/preprocessing.pyapp/analysis/url/analyzer.pyapp/analysis/url/tracker.pyapp/chat/schemas.pyapp/chat/service.pyapp/core/config.pyapp/infrastructure/errors.pyapp/infrastructure/gemini/client.pyapp/infrastructure/google_safe_browsing/client.pyapp/infrastructure/http_retry.pyapp/infrastructure/mock_provider.pyapp/infrastructure/rabbitmq/connection.pyapp/infrastructure/rabbitmq/consumer.pyapp/infrastructure/rabbitmq/dead_letter.pyapp/infrastructure/rabbitmq/handler.pyapp/infrastructure/rabbitmq/publisher.pyapp/infrastructure/rabbitmq/result_factory.pyapp/infrastructure/rabbitmq/schemas.pyapp/infrastructure/virustotal/client.pyapp/main.pydata_science/SMSModel/README.mddata_science/SMSModel/SMSDataModel.ipynbdata_science/SMSModel/dataset_splitting/__init__.pydata_science/SMSModel/dataset_splitting/manifest.pydata_science/SMSModel/dataset_splitting/splitter.pydata_science/SMSModel/dataset_splitting/validation.pydata_science/SMSModel/evaluation/__init__.pydata_science/SMSModel/evaluation/evaluator.pydata_science/SMSModel/evaluation/latency.pydata_science/SMSModel/evaluation/metrics.pydata_science/SMSModel/evaluation/reporting.pydata_science/SMSModel/evaluation/threshold.pydata_science/SMSModel/modeling/__init__.pydata_science/SMSModel/modeling/artifacts.pydata_science/SMSModel/modeling/base.pydata_science/SMSModel/modeling/naive_bayes.pydata_science/SMSModel/reporting/__init__.pydata_science/SMSModel/reporting/dataset_split_report.pydata_science/SMSModel/run_naive_bayes_baseline.pydata_science/SMSModel/template_grouping/config.pydata_science/SMSModel/template_grouping/fingerprint.pydata_science/SMSModel/template_grouping/similarity.pydata_science/SMSModel/tokenization/__init__.pydata_science/SMSModel/tokenization/kiwi_tokenizer.pydata_science/SMSModel/train_sms.pydata_science/VoiceModel/VoiceDataModel.ipynbdata_science/VoiceModel/train_voice.pytests/analysis/rules/test_analyzer.pytests/analysis/test_execution.pytests/analysis/test_router.pytests/analysis/test_scoring.pytests/analysis/test_service.pytests/analysis/text/test_gemini_analyzer.pytests/analysis/text/test_naive_bayes_analyzer.pytests/analysis/text/test_nb_masking_accuracy.pytests/analysis/text/test_preprocessing.pytests/analysis/url/test_analyzer.pytests/analysis/url/test_extractor.pytests/analysis/url/test_tracker.pytests/chat/test_router.pytests/chat/test_service.pytests/data_science/SMSModel/evaluation/conftest.pytests/data_science/SMSModel/evaluation/test_threshold.pytests/data_science/SMSModel/modeling/test_artifacts.pytests/data_science/SMSModel/modeling/test_baseline_runner.pytests/data_science/SMSModel/modeling/test_naive_bayes.pytests/data_science/SMSModel/test_dataset_split_report.pytests/data_science/SMSModel/test_dataset_splitting.pytests/data_science/SMSModel/test_template_grouping.pytests/data_science/SMSModel/tokenization/__init__.pytests/data_science/SMSModel/tokenization/test_kiwi_tokenizer.pytests/infrastructure/rabbitmq/test_connection.pytests/infrastructure/rabbitmq/test_consumer.pytests/infrastructure/rabbitmq/test_dead_letter.pytests/infrastructure/rabbitmq/test_handler.pytests/infrastructure/rabbitmq/test_lifecycle.pytests/infrastructure/rabbitmq/test_publisher.pytests/infrastructure/rabbitmq/test_result_factory.pytests/infrastructure/rabbitmq/test_result_schemas.pytests/infrastructure/rabbitmq/test_schemas.pytests/infrastructure/test_http_retry.pytests/infrastructure/test_virustotal_client.pytests/integration/test_url_tracker.py
💤 Files with no reviewable changes (2)
- data_science/SMSModel/template_grouping/config.py
- tests/data_science/SMSModel/evaluation/conftest.py
🚧 Files skipped from review as they are similar to previous changes (28)
- tests/data_science/SMSModel/evaluation/test_threshold.py
- data_science/SMSModel/modeling/init.py
- data_science/SMSModel/reporting/init.py
- Dockerfile
- tests/data_science/SMSModel/tokenization/init.py
- tests/analysis/text/test_preprocessing.py
- data_science/SMSModel/dataset_splitting/init.py
- data_science/SMSModel/tokenization/init.py
- data_science/SMSModel/dataset_splitting/validation.py
- tests/data_science/SMSModel/modeling/test_naive_bayes.py
- tests/data_science/SMSModel/modeling/test_baseline_runner.py
- data_science/SMSModel/evaluation/metrics.py
- data_science/SMSModel/evaluation/init.py
- data_science/SMSModel/template_grouping/fingerprint.py
- tests/data_science/SMSModel/test_dataset_split_report.py
- data_science/SMSModel/dataset_splitting/splitter.py
- data_science/SMSModel/run_naive_bayes_baseline.py
- tests/analysis/text/test_naive_bayes_analyzer.py
- app/core/config.py
- data_science/SMSModel/modeling/base.py
- data_science/SMSModel/evaluation/evaluator.py
- data_science/SMSModel/evaluation/threshold.py
- tests/data_science/SMSModel/test_template_grouping.py
- data_science/SMSModel/tokenization/kiwi_tokenizer.py
- data_science/SMSModel/template_grouping/similarity.py
- app/analysis/text/preprocessing.py
- data_science/SMSModel/evaluation/latency.py
- data_science/SMSModel/evaluation/reporting.py
Comment on lines
+526
to
+541
| if ( | ||
| rec_p >= TARGET_PHISHING_RECALL | ||
| and rec_n > best["recall_normal"] | ||
| or best["model"] is None | ||
| and rec_p > best["recall_phishing"] | ||
| ): | ||
| best.update( | ||
| { | ||
| "model": model, | ||
| "model_name": model_cls.__name__, | ||
| "alpha": alpha, | ||
| "threshold": threshold, | ||
| "recall_phishing": rec_p, | ||
| "recall_normal": rec_n, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prioritize target-recall candidates over fallback candidates.
The current condition can retain a model below TARGET_PHISHING_RECALL when it has higher recall_normal than a later target-meeting model. This violates the documented selection order.
Proposed fix
- if (
- rec_p >= TARGET_PHISHING_RECALL
- and rec_n > best["recall_normal"]
- or best["model"] is None
- and rec_p > best["recall_phishing"]
- ):
+ candidate_meets_target = rec_p >= TARGET_PHISHING_RECALL
+ best_meets_target = (
+ best["model"] is not None
+ and best["recall_phishing"] >= TARGET_PHISHING_RECALL
+ )
+ if (
+ best["model"] is None
+ or (
+ candidate_meets_target
+ and (
+ not best_meets_target
+ or rec_n > best["recall_normal"]
+ )
+ )
+ or (
+ not candidate_meets_target
+ and not best_meets_target
+ and rec_p > best["recall_phishing"]
+ )
+ ):📝 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.
Suggested change
| if ( | |
| rec_p >= TARGET_PHISHING_RECALL | |
| and rec_n > best["recall_normal"] | |
| or best["model"] is None | |
| and rec_p > best["recall_phishing"] | |
| ): | |
| best.update( | |
| { | |
| "model": model, | |
| "model_name": model_cls.__name__, | |
| "alpha": alpha, | |
| "threshold": threshold, | |
| "recall_phishing": rec_p, | |
| "recall_normal": rec_n, | |
| } | |
| ) | |
| candidate_meets_target = rec_p >= TARGET_PHISHING_RECALL | |
| best_meets_target = ( | |
| best["model"] is not None | |
| and best["recall_phishing"] >= TARGET_PHISHING_RECALL | |
| ) | |
| if ( | |
| best["model"] is None | |
| or ( | |
| candidate_meets_target | |
| and ( | |
| not best_meets_target | |
| or rec_n > best["recall_normal"] | |
| ) | |
| ) | |
| or ( | |
| not candidate_meets_target | |
| and not best_meets_target | |
| and rec_p > best["recall_phishing"] | |
| ) | |
| ): | |
| best.update( | |
| { | |
| "model": model, | |
| "model_name": model_cls.__name__, | |
| "alpha": alpha, | |
| "threshold": threshold, | |
| "recall_phishing": rec_p, | |
| "recall_normal": rec_n, | |
| } | |
| ) |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 527-528: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
[warning] 529-530: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
🤖 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/VoiceModel/train_voice.py` around lines 526 - 541, Update the
model-selection condition in the training loop so candidates meeting
TARGET_PHISHING_RECALL always take precedence over fallback candidates,
regardless of recall_normal. Only use the best["model"] is None fallback when no
target-recall candidate has been selected, while preserving the existing metric
updates in best.update.
Comment on lines
+167
to
+182
| "raw_scores = (prob_phishing * 100).astype(int)\n", | ||
| "risk_scores = np.array(\n", | ||
| " [\n", | ||
| " tv._apply_risk_floor(s, struct[i], df_eval[\"text_clean\"].iloc[i])\n", | ||
| " for i, s in enumerate(raw_scores)\n", | ||
| " ]\n", | ||
| ")\n", | ||
| "\n", | ||
| "df_eval[\"prob_phishing\"] = prob_phishing\n", | ||
| "df_eval[\"risk_score\"] = risk_scores\n", | ||
| "df_eval[\"risk_level\"] = df_eval[\"risk_score\"].apply(tv._map_risk_level)\n", | ||
| "df_eval[\"risk_score\"] = risk_scores\n", | ||
| "df_eval[\"risk_level\"] = df_eval[\"risk_score\"].apply(tv._map_risk_level)\n", | ||
| "\n", | ||
| "print(f\"[Score] test+holdout {len(df_eval)}건 채점 완료 | \"\n", | ||
| " f\"label={df_eval['label'].value_counts().to_dict()}\")\n" | ||
| "print(\n", | ||
| " f\"[Score] test+holdout {len(df_eval)}건 채점 완료 | \"\n", | ||
| " f\"label={df_eval['label'].value_counts().to_dict()}\"\n", | ||
| ")" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore a runnable artifact and feature schema.
This scoring cell has saved output that shows X has 8010 features, but ComplementNB is expecting 8008 features as input. The cell cannot produce risk_score values.
Rebuild matching model and vectorizer artifacts, then rerun the notebook. Add a feature-schema compatibility check before model.predict_proba(X) so stale artifacts fail with a direct remediation message.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 174-174: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 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/VoiceModel/VoiceDataModel.ipynb` around lines 167 - 182, Rebuild
and save matching ComplementNB model and vectorizer artifacts, then rerun the
notebook so the scoring cell produces valid risk_score values. In the scoring
flow before model.predict_proba(X), add a feature-count compatibility check
between X and the model’s expected input features, raising a direct message
instructing users to regenerate matching artifacts when they differ.
This was referenced Aug 9, 2026
This was referenced Aug 19, 2026
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.
📝 개요
이 PR은 이슈 #36을 두 개의 PR로 나누어 진행하는 첫 번째 PR입니다.
기존 Naive Bayes 모델과 형태소 기반 Logistic Regression, 문자 n-gram
기반 Linear SVM을 동일한 조건에서 비교하기 위해서는 먼저 재현 가능한
데이터 분할과 공통 평가 기반이 필요합니다.
Logistic Regression, Linear SVM 및 최종 모델 비교는 후속 PR
(2/2)에서구현할 예정입니다.
🔗 관련 이슈
🎯 주요 변경 사항
1. 공통 SMS 전처리
2. 중복·유사 메시지 그룹화
template_group_id부여3. 데이터 누수 방지 분할
template_group_id단위 train/validation/test 분할생성 파일:
data_science/SMSModel/splits/sms_split_v1.csv4. 데이터 분할 검증 보고서
생성 파일:
data_science/SMSModel/reports/dataset_split_summary.jsondata_science/SMSModel/reports/dataset_split_summary.md5. 공통 모델 평가 파이프라인
다음 공통 인터페이스와 평가 기능을 추가했습니다.
fitpredictpredict_scores6. 기존 Naive Bayes baseline 연결
ComplementNB설정 유지평가 결과:
평가 보고서:
data_science/SMSModel/reports/model_evaluation/naive_bayes_baseline/7. Kiwi 형태소 tokenizer
kiwipiepy==0.23.2의존성 추가[URL],[ACCOUNT]등 마스킹 토큰 보존CountVectorizer연동 테스트데이터 분할 결과
1db45d5f2c3d3d17726888b05cd625e0d0a51deef3dc8ab94016a9ee97af18f5📸 사진
✅ PR 체크리스트
uvicorn구동 또는 테스트 코드)를 통과했습니다.Summary by CodeRabbit