Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces the dialog normalizer's inline regex-based normalization with a new locale-aware ChangesNormalization rewrite
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Transformer as DialogNormalizerTransformer
participant Normalize as normalize()
participant LocaleData as LocaleDataManager
participant Parsers as ovos_number_parser / ovos_date_parser / unicode_rbnf
Transformer->>Normalize: normalize(original, sess.lang)
Normalize->>Normalize: _normalize_dates_and_times(text)
Normalize->>Parsers: pronounce_date/pronounce_time
Normalize->>Normalize: _normalize_word_hyphen_digit(text)
Normalize->>LocaleData: get_units(lang)
Normalize->>Parsers: pronounce number for unit match
Normalize->>LocaleData: get_contractions(lang), get_titles(lang)
Normalize->>Parsers: _normalize_number_word per token
Normalize-->>Transformer: normalized text
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Docstrings generation was requested by @JarbasAl. * #2 (comment) The following files were modified: * `ovos_dialog_normalizer_plugin/__init__.py` * `ovos_dialog_normalizer_plugin/util.py`
|
Note Generated docstrings for this pull request at #3 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
requirements.txt (1)
2-5: Consider pinning versions for new dependenciesThe newly added dependencies
langcodesandunicode_rbnfdon't have version constraints. To ensure reproducible builds and avoid unexpected breaking changes, consider adding version constraints.ovos-plugin-manager -langcodes +langcodes>=3.3.0 ovos-number-parser>=0.4.0 ovos-date-parser>=0.6.4a1 -unicode_rbnf +unicode_rbnf>=1.1.0ovos_dialog_normalizer_plugin/util.py (3)
14-186: Add contractions for other supported languagesThe CONTRACTIONS dictionary only includes English entries, but the module claims to support multiple languages (pt, es, fr, de, etc.). Consider adding common contractions for these languages to ensure consistent normalization across all supported languages.
Would you like me to help generate common contractions for Portuguese, Spanish, French, and other supported languages?
500-506: Document year disambiguation logicThe 2-digit year disambiguation logic assumes years 00-29 map to 2000-2029 and 30-99 map to 1930-1999. This assumption may not be appropriate for all use cases and could lead to incorrect date parsing.
Consider making this configurable or documenting this behavior clearly in the function docstring:
def _normalize_dates_and_times(text: str, full_lang: str, date_format: str = "DMY") -> str: """ Helper function to normalize dates and times using regular expressions. This prepares the strings for pronunciation. + + Note: 2-digit years are expanded as follows: + - 00-29 -> 2000-2029 + - 30-99 -> 1930-1999 """
1-719: Consider future modularization for maintainabilityThis utility module is comprehensive but quite large (700+ lines). As the normalization features grow, consider splitting it into smaller, focused modules:
contractions.py- Language-specific contraction mappingsnumbers.py- Number and fraction normalizationdatetime.py- Date and time normalizationunits.py- Unit conversion and normalizationThis would improve maintainability and make it easier to add language-specific features.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ovos_dialog_normalizer_plugin/__init__.py(2 hunks)ovos_dialog_normalizer_plugin/util.py(1 hunks)requirements.txt(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
ovos_dialog_normalizer_plugin/util.py
150-150: Dictionary key literal "shan't" repeated
Remove repeated key literal "shan't"
(F601)
712-712: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (1)
ovos_dialog_normalizer_plugin/__init__.py (1)
6-27: Clean refactoring with good separation of concerns!The extraction of normalization logic to a dedicated utility module improves maintainability and makes the transformer class focused on its plugin responsibilities. Error handling and logging are properly preserved.
Docstrings generation was requested by @JarbasAl. * #2 (comment) The following files were modified: * `ovos_dialog_normalizer_plugin/__init__.py` * `ovos_dialog_normalizer_plugin/util.py` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
ovos_dialog_normalizer_plugin/util.py (3)
12-12: Use a more specific logger nameThe logger name "normalize" is too generic and could conflict with other modules. Consider using a name that includes the package namespace.
150-150: Remove duplicate dictionary keyThe key
"shan't"is already defined at line 51. This duplicate at line 150 will override the previous value.
772-772: Remove unnecessary f-string prefixThis string doesn't contain any placeholders, so the
fprefix is not needed.
🧹 Nitpick comments (3)
ovos_dialog_normalizer_plugin/util.py (3)
362-378: Consider using locale information for separator detectionWhile the current implementation works for common languages, consider using Python's
localemodule or thebabellibrary for more comprehensive and accurate locale-specific number formatting.Example using babel:
from babel import Locale def _get_number_separators(full_lang: str) -> tuple[str, str]: try: locale = Locale.parse(full_lang.replace('-', '_')) decimal_separator = locale.number_symbols.get('decimal', '.') thousands_separator = locale.number_symbols.get('group', ',') return decimal_separator, thousands_separator except Exception: # Fallback to current implementation lang_code = full_lang.split("-")[0] if lang_code in ["pt", "es", "fr", "de"]: return ',', '.' return '.', ','
531-534: Document the year expansion logicThe 2-digit year expansion logic assumes years 00-29 map to 2000-2029 and 30-99 map to 1930-1999. This assumption should be documented and might need to be configurable in the future.
# Expand 2-digit year to 4-digit year if year < 100: - # Assume years 00-29 are 2000-2029, 30-99 are 1930-1999 + # Assume years 00-29 are 2000-2029, 30-99 are 1930-1999 + # TODO: Consider making this cutoff configurable or date-aware year = 2000 + year if year < 30 else 1900 + year
1-779: Successfully ported normalization with improvementsThis implementation successfully ports the normalization functionality from the phoonnx project while adding comprehensive error handling, multi-language support, and modular design. The code is well-structured and maintains good separation of concerns.
Consider adding unit tests to ensure the normalization behavior remains consistent across updates, especially for edge cases like 2-digit year expansion and locale-specific number formatting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ovos_dialog_normalizer_plugin/__init__.py(2 hunks)ovos_dialog_normalizer_plugin/util.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ovos_dialog_normalizer_plugin/init.py
🧰 Additional context used
🪛 Ruff (0.12.2)
ovos_dialog_normalizer_plugin/util.py
150-150: Dictionary key literal "shan't" repeated
Remove repeated key literal "shan't"
(F601)
772-772: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (5)
ovos_dialog_normalizer_plugin/util.py (5)
188-359: Well-structured multi-language dictionariesThe TITLES and UNITS dictionaries provide comprehensive coverage for multiple languages with clear organization.
381-436: Robust number normalization with proper error handlingThe implementation correctly handles various number formats, locale-specific separators, and provides appropriate fallback mechanisms with error logging.
440-467: Clean date/time pronunciation interfacesBoth functions provide appropriate abstractions with good error handling, especially the fallback mechanism in
pronounce_time.
563-572: Simple and effective hyphen-digit normalizationThe regex pattern correctly handles word-hyphen-digit combinations.
693-729: Well-orchestrated normalization pipelineThe main normalize function properly coordinates all normalization steps in a logical order with appropriate error handling for RBNF engine initialization.
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ovos_dialog_normalizer_plugin/util.py (1)
484-484: Remove unnecessary f-string prefixThis string doesn't contain any placeholders, so the
fprefix is not needed.- print(f"\n--- Other Examples ---") + print("\n--- Other Examples ---")
🧹 Nitpick comments (3)
setup.py (1)
69-70: Consider being more selective with package data inclusionIncluding all files from the plugin directory might package unnecessary files (e.g.,
__pycache__,.pycfiles). Consider filtering by file extensions or using a MANIFEST.in file.def package_files(directory): paths = [] for (path, _, filenames) in os.walk(directory): for filename in filenames: - paths.append(os.path.join(path, filename)) + # Only include JSON files and other necessary resources + if filename.endswith(('.json', '.txt', '.yml', '.yaml')): + paths.append(os.path.join(path, filename)) return pathsAlternatively, use a MANIFEST.in file for better control over included files.
ovos_dialog_normalizer_plugin/util.py (2)
79-83: Consider implementing the TODO: Move separator logic to locale JSON filesThe hardcoded language-specific separator logic could be moved to JSON files for better maintainability and extensibility.
Would you like me to help implement this by:
- Creating a JSON structure for number format configurations
- Updating the LocaleDataManager to load this data
- Refactoring
_get_number_separatorsto use the JSON dataThis would make it easier to add support for new languages without modifying code.
303-337: Cache compiled regex patterns for better performanceThe regex patterns are compiled on every function call. For better performance, especially when processing multiple texts, consider caching the compiled patterns.
# Add to LocaleDataManager or create a separate cache class RegexCache: def __init__(self): self._cache = {} def get_units_regex(self, lang_code, separator_info): cache_key = (lang_code, separator_info) if cache_key not in self._cache: # Build and compile patterns self._cache[cache_key] = self._build_units_patterns(lang_code, separator_info) return self._cache[cache_key]This would significantly improve performance when normalizing multiple texts in the same language.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.github/workflows/unit_tests.yml(1 hunks)ovos_dialog_normalizer_plugin/locale/ca/titles.json(1 hunks)ovos_dialog_normalizer_plugin/locale/de/titles.json(1 hunks)ovos_dialog_normalizer_plugin/locale/de/units.json(1 hunks)ovos_dialog_normalizer_plugin/locale/en/contractions.json(1 hunks)ovos_dialog_normalizer_plugin/locale/en/titles.json(1 hunks)ovos_dialog_normalizer_plugin/locale/en/units.json(1 hunks)ovos_dialog_normalizer_plugin/locale/es/titles.json(1 hunks)ovos_dialog_normalizer_plugin/locale/es/units.json(1 hunks)ovos_dialog_normalizer_plugin/locale/fr/titles.json(1 hunks)ovos_dialog_normalizer_plugin/locale/fr/units.json(1 hunks)ovos_dialog_normalizer_plugin/locale/gl/titles.json(1 hunks)ovos_dialog_normalizer_plugin/locale/it/titles.json(1 hunks)ovos_dialog_normalizer_plugin/locale/nl/titles.json(1 hunks)ovos_dialog_normalizer_plugin/locale/pt/titles.json(1 hunks)ovos_dialog_normalizer_plugin/locale/pt/units.json(1 hunks)ovos_dialog_normalizer_plugin/util.py(1 hunks)requirements.txt(1 hunks)setup.py(2 hunks)tests/__init__.py(1 hunks)
✅ Files skipped from review due to trivial changes (17)
- ovos_dialog_normalizer_plugin/locale/ca/titles.json
- tests/init.py
- ovos_dialog_normalizer_plugin/locale/es/titles.json
- ovos_dialog_normalizer_plugin/locale/de/titles.json
- ovos_dialog_normalizer_plugin/locale/en/titles.json
- ovos_dialog_normalizer_plugin/locale/nl/titles.json
- ovos_dialog_normalizer_plugin/locale/it/titles.json
- ovos_dialog_normalizer_plugin/locale/es/units.json
- ovos_dialog_normalizer_plugin/locale/fr/units.json
- ovos_dialog_normalizer_plugin/locale/de/units.json
- ovos_dialog_normalizer_plugin/locale/fr/titles.json
- ovos_dialog_normalizer_plugin/locale/gl/titles.json
- ovos_dialog_normalizer_plugin/locale/pt/units.json
- ovos_dialog_normalizer_plugin/locale/en/units.json
- ovos_dialog_normalizer_plugin/locale/pt/titles.json
- .github/workflows/unit_tests.yml
- ovos_dialog_normalizer_plugin/locale/en/contractions.json
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 Additional context used
🪛 Ruff (0.12.2)
setup.py
53-53: Loop control variable directories not used within loop body
Rename unused directories to _directories
(B007)
ovos_dialog_normalizer_plugin/util.py
484-484: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (1)
ovos_dialog_normalizer_plugin/util.py (1)
236-240: Review 2-digit year expansion logicThe current logic assumes years 00-29 map to 2000-2029 and 30-99 map to 1930-1999. Since we're in 2025, this might misinterpret recent dates.
Consider making the cutoff year configurable or using a sliding window approach. For example, dates within 50 years in the past and 30 years in the future from the current year.
# Example sliding window approach current_year = datetime.now().year if year < 100: # Assume within -50 to +30 years from current year century = (current_year // 100) * 100 candidate_year = century + year if candidate_year > current_year + 30: candidate_year -= 100 elif candidate_year < current_year - 50: candidate_year += 100 year = candidate_year
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
Port the text normalization pipeline: expand contractions, titles, numbers, fractions, units, dates and times into their spoken forms for TTS, driven by per-language JSON locale data (ca, de, en, es, fr, gl, it, nl, pt). Only expand am/pm when attached to a clock number so unrelated words are left untouched. Adds unit tests and wires the unit-test workflow; bundles locale data in the wheel. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ping! I've got your results right here. 🛎️I've aggregated the results of the automated checks for this PR below. 🏷️ Release PreviewPolishing the release notes for the big debut! 🎀 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
📋 Repo HealthA thorough inspection of the project's hygiene. 🧼 Latest Version: ✅ ⚖️ License CheckI've checked the genealogical tree of your licenses. 🌳 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔍 LintThe data is in, and it's looking interesting! 🧐 ❌ ruff: issues found — see job log Processing completed in 0.0001 bot-seconds ⚡ |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
test/test_normalize.py (1)
18-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests assert exact third-party library output strings.
These tests hardcode exact pronunciation strings produced by
ovos_number_parser/unicode_rbnf(e.g. commas, "and" placement, unit words). The dependencies inpyproject.tomlallow minor version bumps (<1.0.0,<3.0.0), so a library update that tweaks formatting (spacing, "and" usage, punctuation) could silently break these tests without any actual regression in this plugin's logic. Consider testing structural properties (e.g. absence of digits, presence of key words) instead of full exact-string equality where feasible, or pin the number/date parser dependencies more tightly.🤖 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 `@test/test_normalize.py` around lines 18 - 48, The normalize tests are overfitting to exact third-party pronunciation output from normalize and its underlying ovos_number_parser/unicode_rbnf dependencies, so they may fail on harmless formatter changes. Update the assertions in TestNormalizeEnglish and TestNormalizePortuguese to check stable structural behavior instead of full-string equality where possible, such as ensuring digits are removed, key terms like Doctor/Professor/kilograms/per cent remain, and hyphen handling works; alternatively, tighten the dependency version constraints if exact output must be preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/unit_tests.yml:
- Around line 31-35: The system dependency install step in the workflow can
prompt interactively and fail in CI. Update the Install System Dependencies step
to use the non-interactive apt-get form with the existing package install block,
and keep the workflow behavior unchanged otherwise. Use the step name and the
apt install command in the workflow to locate the change.
- Line 26: The workflow currently uses actions/checkout without disabling
persisted credentials, so the GitHub token remains in git config for later
PR-controlled steps. Update the checkout step in the unit_tests workflow to
prevent credential persistence before running pip install -e .[test] and pytest,
using the checkout configuration that disables stored credentials. Keep the fix
anchored around the existing actions/checkout@v4 step.
In `@ovos_dialog_normalizer_plugin/util.py`:
- Around line 218-263: The date normalization logic in util.py only processes
the first match because it uses date_pattern.search(text), so later dates in the
same utterance are skipped. Update the date-handling flow in the same
normalization function to iterate over every match from date_pattern and replace
each one, preserving the existing parsing and pronounce_date/date() behavior
while ensuring all date occurrences are normalized.
- Around line 322-325: The unit normalization path in the regex match handler
uses case-sensitive dictionary lookup even though the pattern is compiled with
re.IGNORECASE, so captured unit symbols can fail for inputs like 25ºc. Update
the lookup in the relevant match-processing logic (the branch that reads
match.group(2) and accesses symbolic_units, including the later similar
handling) to normalize the captured unit symbol before indexing, and keep the
rest of the pronunciation flow in that same function intact.
- Around line 125-139: The numeric fallback in util.py returns too early from
the is_numeric(temp_cleaned_word) branch, preventing the rbnf_engine path from
handling digit words when pronounce_number fails. Update the logic around the
number-pronunciation flow so the exception in the is_numeric branch falls
through to the existing rbnf_engine and cleaned_word.isdigit() fallback instead
of immediately returning the original word, preserving the suffix handling with
word[len(cleaned_word):] in both paths.
- Around line 29-32: Validate lang_code in _load_data before constructing
file_path, since normalize(text, lang) can pass public input into the filesystem
join. Add a strict guard in _load_data to reject unexpected locale components
(for example, path separators, traversal, or other non-locales) before
os.path.join(RESOURCES_DIR, lang_code, ...) is used. Keep the fix localized to
_load_data and preserve the existing caching/loading flow once the lang_code
check passes.
---
Nitpick comments:
In `@test/test_normalize.py`:
- Around line 18-48: The normalize tests are overfitting to exact third-party
pronunciation output from normalize and its underlying
ovos_number_parser/unicode_rbnf dependencies, so they may fail on harmless
formatter changes. Update the assertions in TestNormalizeEnglish and
TestNormalizePortuguese to check stable structural behavior instead of
full-string equality where possible, such as ensuring digits are removed, key
terms like Doctor/Professor/kilograms/per cent remain, and hyphen handling
works; alternatively, tighten the dependency version constraints if exact output
must be preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70fb947b-8928-49ed-8d8f-baba6a03ad87
📒 Files selected for processing (22)
.github/workflows/unit_tests.yml.gitignoreREADME.mdovos_dialog_normalizer_plugin/__init__.pyovos_dialog_normalizer_plugin/locale/ca/titles.jsonovos_dialog_normalizer_plugin/locale/de/titles.jsonovos_dialog_normalizer_plugin/locale/de/units.jsonovos_dialog_normalizer_plugin/locale/en/contractions.jsonovos_dialog_normalizer_plugin/locale/en/titles.jsonovos_dialog_normalizer_plugin/locale/en/units.jsonovos_dialog_normalizer_plugin/locale/es/titles.jsonovos_dialog_normalizer_plugin/locale/es/units.jsonovos_dialog_normalizer_plugin/locale/fr/titles.jsonovos_dialog_normalizer_plugin/locale/fr/units.jsonovos_dialog_normalizer_plugin/locale/gl/titles.jsonovos_dialog_normalizer_plugin/locale/it/titles.jsonovos_dialog_normalizer_plugin/locale/nl/titles.jsonovos_dialog_normalizer_plugin/locale/pt/titles.jsonovos_dialog_normalizer_plugin/locale/pt/units.jsonovos_dialog_normalizer_plugin/util.pypyproject.tomltest/test_normalize.py
✅ Files skipped from review due to trivial changes (12)
- .gitignore
- ovos_dialog_normalizer_plugin/locale/de/titles.json
- ovos_dialog_normalizer_plugin/locale/pt/titles.json
- ovos_dialog_normalizer_plugin/locale/fr/titles.json
- ovos_dialog_normalizer_plugin/locale/en/contractions.json
- ovos_dialog_normalizer_plugin/locale/gl/titles.json
- ovos_dialog_normalizer_plugin/locale/nl/titles.json
- ovos_dialog_normalizer_plugin/locale/es/titles.json
- ovos_dialog_normalizer_plugin/locale/it/titles.json
- ovos_dialog_normalizer_plugin/locale/de/units.json
- ovos_dialog_normalizer_plugin/locale/pt/units.json
- README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- ovos_dialog_normalizer_plugin/locale/en/titles.json
- ovos_dialog_normalizer_plugin/locale/en/units.json
- ovos_dialog_normalizer_plugin/locale/ca/titles.json
- ovos_dialog_normalizer_plugin/locale/fr/units.json
- ovos_dialog_normalizer_plugin/locale/es/units.json
- ovos_dialog_normalizer_plugin/init.py
| python-version: [ "3.10", "3.11", "3.12" ] | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable persisted checkout credentials before running PR-controlled steps.
pip install -e .[test] and pytest run after checkout, so the workflow should avoid leaving the GitHub token in git config.
🔒 Proposed hardening
- - uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 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.
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 26-26: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/unit_tests.yml at line 26, The workflow currently uses
actions/checkout without disabling persisted credentials, so the GitHub token
remains in git config for later PR-controlled steps. Update the checkout step in
the unit_tests workflow to prevent credential persistence before running pip
install -e .[test] and pytest, using the checkout configuration that disables
stored credentials. Keep the fix anchored around the existing
actions/checkout@v4 step.
Source: Linters/SAST tools
| - name: Install System Dependencies | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt install python3-dev | ||
| python -m pip install build wheel |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make apt installation non-interactive.
apt install can prompt in CI and fail without a TTY; use apt-get install -y.
🛠️ Proposed fix
- sudo apt install python3-dev
+ sudo apt-get install -y python3-dev📝 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.
| - name: Install System Dependencies | |
| run: | | |
| sudo apt-get update | |
| sudo apt install python3-dev | |
| python -m pip install build wheel | |
| - name: Install System Dependencies | |
| run: | | |
| sudo apt-get update | |
| sudo apt-get install -y python3-dev | |
| python -m pip install build wheel |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/unit_tests.yml around lines 31 - 35, The system dependency
install step in the workflow can prompt interactively and fail in CI. Update the
Install System Dependencies step to use the non-interactive apt-get form with
the existing package install block, and keep the workflow behavior unchanged
otherwise. Use the step name and the apt install command in the workflow to
locate the change.
| def _load_data(self, lang_code: str, file_name: str) -> dict: | ||
| """Loads a single JSON file and caches it.""" | ||
| file_path = os.path.join(RESOURCES_DIR, lang_code, f"{file_name}.json") | ||
| try: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate lang_code before using it in a filesystem path.
lang_code comes from the public normalize(text, lang) path and is joined into RESOURCES_DIR; reject unexpected locale components before opening JSON files.
🔒 Proposed guard
+_LANG_CODE_RE = re.compile(r"^[a-zA-Z]{2,3}$")
+
+
class LocaleDataManager:
@@
def _load_data(self, lang_code: str, file_name: str) -> dict:
"""Loads a single JSON file and caches it."""
+ if not _LANG_CODE_RE.fullmatch(lang_code):
+ LOG.debug(f"Unsupported locale code: {lang_code!r}. Using empty dictionary.")
+ self.cache.setdefault(lang_code, {})[file_name] = {}
+ return {}
file_path = os.path.join(RESOURCES_DIR, lang_code, f"{file_name}.json")📝 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.
| def _load_data(self, lang_code: str, file_name: str) -> dict: | |
| """Loads a single JSON file and caches it.""" | |
| file_path = os.path.join(RESOURCES_DIR, lang_code, f"{file_name}.json") | |
| try: | |
| _LANG_CODE_RE = re.compile(r"^[a-zA-Z]{2,3}$") | |
| class LocaleDataManager: | |
| def _load_data(self, lang_code: str, file_name: str) -> dict: | |
| """Loads a single JSON file and caches it.""" | |
| if not _LANG_CODE_RE.fullmatch(lang_code): | |
| LOG.debug(f"Unsupported locale code: {lang_code!r}. Using empty dictionary.") | |
| self.cache.setdefault(lang_code, {})[file_name] = {} | |
| return {} | |
| file_path = os.path.join(RESOURCES_DIR, lang_code, f"{file_name}.json") | |
| try: |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 32-32: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(file_path, "r", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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 `@ovos_dialog_normalizer_plugin/util.py` around lines 29 - 32, Validate
lang_code in _load_data before constructing file_path, since normalize(text,
lang) can pass public input into the filesystem join. Add a strict guard in
_load_data to reject unexpected locale components (for example, path separators,
traversal, or other non-locales) before os.path.join(RESOURCES_DIR, lang_code,
...) is used. Keep the fix localized to _load_data and preserve the existing
caching/loading flow once the lang_code check passes.
Source: Linters/SAST tools
| if is_numeric(temp_cleaned_word): | ||
| try: | ||
| num = float(temp_cleaned_word) if "." in temp_cleaned_word else int(temp_cleaned_word) | ||
| return pronounce_number(num, lang=full_lang) + word[len(cleaned_word):] | ||
| except Exception as e: | ||
| LOG.error(f"ovos-number-parser failed to pronounce number: {word} - ({e})") | ||
| return word | ||
|
|
||
| elif rbnf_engine and cleaned_word.isdigit(): | ||
| try: | ||
| pronounced_number = rbnf_engine.format_number(cleaned_word, FormatPurpose.CARDINAL).text | ||
| return pronounced_number + word[len(cleaned_word):] | ||
| except Exception as e: | ||
| LOG.error(f"unicode-rbnf failed to pronounce number: {word} - ({e})") | ||
| return word |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Let RBNF handle numeric fallback failures.
For digit words, is_numeric(...) is true, so a pronounce_number exception returns the original word before the RBNF branch can run.
🛠️ Proposed fallback
try:
num = float(temp_cleaned_word) if "." in temp_cleaned_word else int(temp_cleaned_word)
return pronounce_number(num, lang=full_lang) + word[len(cleaned_word):]
except Exception as e:
LOG.error(f"ovos-number-parser failed to pronounce number: {word} - ({e})")
- return word
+ if rbnf_engine and cleaned_word.isdigit():
+ try:
+ pronounced_number = rbnf_engine.format_number(cleaned_word, FormatPurpose.CARDINAL).text
+ return pronounced_number + word[len(cleaned_word):]
+ except Exception as rbnf_error:
+ LOG.error(f"unicode-rbnf failed to pronounce number: {word} - ({rbnf_error})")
+ return word
- elif rbnf_engine and cleaned_word.isdigit():
+ if rbnf_engine and cleaned_word.isdigit():📝 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.
| if is_numeric(temp_cleaned_word): | |
| try: | |
| num = float(temp_cleaned_word) if "." in temp_cleaned_word else int(temp_cleaned_word) | |
| return pronounce_number(num, lang=full_lang) + word[len(cleaned_word):] | |
| except Exception as e: | |
| LOG.error(f"ovos-number-parser failed to pronounce number: {word} - ({e})") | |
| return word | |
| elif rbnf_engine and cleaned_word.isdigit(): | |
| try: | |
| pronounced_number = rbnf_engine.format_number(cleaned_word, FormatPurpose.CARDINAL).text | |
| return pronounced_number + word[len(cleaned_word):] | |
| except Exception as e: | |
| LOG.error(f"unicode-rbnf failed to pronounce number: {word} - ({e})") | |
| return word | |
| if is_numeric(temp_cleaned_word): | |
| try: | |
| num = float(temp_cleaned_word) if "." in temp_cleaned_word else int(temp_cleaned_word) | |
| return pronounce_number(num, lang=full_lang) + word[len(cleaned_word):] | |
| except Exception as e: | |
| LOG.error(f"ovos-number-parser failed to pronounce number: {word} - ({e})") | |
| if rbnf_engine and cleaned_word.isdigit(): | |
| try: | |
| pronounced_number = rbnf_engine.format_number(cleaned_word, FormatPurpose.CARDINAL).text | |
| return pronounced_number + word[len(cleaned_word):] | |
| except Exception as rbnf_error: | |
| LOG.error(f"unicode-rbnf failed to pronounce number: {word} - ({rbnf_error})") | |
| return word | |
| if rbnf_engine and cleaned_word.isdigit(): | |
| try: | |
| pronounced_number = rbnf_engine.format_number(cleaned_word, FormatPurpose.CARDINAL).text | |
| return pronounced_number + word[len(cleaned_word):] | |
| except Exception as e: | |
| LOG.error(f"unicode-rbnf failed to pronounce number: {word} - ({e})") | |
| return word |
🤖 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 `@ovos_dialog_normalizer_plugin/util.py` around lines 125 - 139, The numeric
fallback in util.py returns too early from the is_numeric(temp_cleaned_word)
branch, preventing the rbnf_engine path from handling digit words when
pronounce_number fails. Update the logic around the number-pronunciation flow so
the exception in the is_numeric branch falls through to the existing rbnf_engine
and cleaned_word.isdigit() fallback instead of immediately returning the
original word, preserving the suffix handling with word[len(cleaned_word):] in
both paths.
| match = date_pattern.search(text) | ||
|
|
||
| if match: | ||
| # Get the three parts of the date string | ||
| part1_str, part2_str, part3_str = match.groups() | ||
| p1, p2, p3 = int(part1_str), int(part2_str), int(part3_str) | ||
|
|
||
| # Initialize month, day, and year | ||
| month, day, year = None, None, None | ||
|
|
||
| # Determine year first based on length (4 digits) | ||
| if len(part1_str) == 4: | ||
| year, rest_parts = p1, [p2, p3] | ||
| elif len(part3_str) == 4: | ||
| year, rest_parts = p3, [p1, p2] | ||
| else: | ||
| # If no 4-digit year, it's ambiguous, assume a 2-digit year. | ||
| # We'll assume the last part is the year based on common patterns. | ||
| year = p3 | ||
| # Expand 2-digit year to 4-digit year | ||
| if year < 100: | ||
| # Assume years 00-29 are 2000-2029, 30-99 are 1930-1999 | ||
| year = 2000 + year if year < 30 else 1900 + year | ||
| rest_parts = [p1, p2] | ||
|
|
||
| # From the remaining parts, try to determine day and month | ||
| if day is None and any(p > 12 and len(str(p)) == 2 for p in rest_parts): | ||
| # If a two-digit number is > 12, it's a day | ||
| day_candidate = next((p for p in rest_parts if p > 12), None) | ||
| if day_candidate: | ||
| day = day_candidate | ||
| rest_parts.remove(day_candidate) | ||
| month = rest_parts[0] | ||
|
|
||
| # Fallback to date_format if day/month are still ambiguous | ||
| if day is None or month is None: | ||
| if date_format.lower() == "mdy": | ||
| month, day = rest_parts[0], rest_parts[1] | ||
| else: # default to DD/MM/YY | ||
| day, month = rest_parts[0], rest_parts[1] | ||
|
|
||
| try: | ||
| date_obj = date(year, month, day) | ||
| pronounced_date_str = pronounce_date(date_obj, full_lang) | ||
| text = text.replace(match.group(0), pronounced_date_str) | ||
| except (ValueError, IndexError) as e: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize every date match, not just the first one.
date_pattern.search(text) only parses one date; utterances with multiple distinct dates leave later dates unnormalized.
🤖 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 `@ovos_dialog_normalizer_plugin/util.py` around lines 218 - 263, The date
normalization logic in util.py only processes the first match because it uses
date_pattern.search(text), so later dates in the same utterance are skipped.
Update the date-handling flow in the same normalization function to iterate over
every match from date_pattern and replace each one, preserving the existing
parsing and pronounce_date/date() behavior while ensuring all date occurrences
are normalized.
| unit_symbol = match.group(2) | ||
| unit_word = symbolic_units[unit_symbol] | ||
| try: | ||
| return f"{pronounce_number(float(number) if '.' in number else int(number), full_lang)} {unit_word}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make unit lookups match the case-insensitive regex.
The regex uses re.IGNORECASE, but lookup uses the captured text as-is. Inputs like 25ºc can miss or raise despite matching the pattern.
🛠️ Proposed fix
symbolic_units = {k: v for k, v in units_data.items() if not k.isalnum()}
alphanumeric_units = {k: v for k, v in units_data.items() if k.isalnum()}
+ symbolic_units_lookup = {k.casefold(): v for k, v in symbolic_units.items()}
+ alphanumeric_units_lookup = {k.casefold(): v for k, v in alphanumeric_units.items()}
@@
unit_symbol = match.group(2)
- unit_word = symbolic_units[unit_symbol]
try:
+ unit_word = symbolic_units_lookup[unit_symbol.casefold()]
return f"{pronounce_number(float(number) if '.' in number else int(number), full_lang)} {unit_word}"
@@
unit_symbol = match.group(2)
try:
- unit_word = alphanumeric_units[unit_symbol]
+ unit_word = alphanumeric_units_lookup[unit_symbol.casefold()]Also applies to: 356-360
🤖 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 `@ovos_dialog_normalizer_plugin/util.py` around lines 322 - 325, The unit
normalization path in the regex match handler uses case-sensitive dictionary
lookup even though the pattern is compiled with re.IGNORECASE, so captured unit
symbols can fail for inputs like 25ºc. Update the lookup in the relevant
match-processing logic (the branch that reads match.group(2) and accesses
symbolic_units, including the later similar handling) to normalize the captured
unit symbol before indexing, and keep the rest of the pronunciation flow in that
same function intact.
1 participant
closes #1
Summary by CodeRabbit
New Features
Bug Fixes
Documentation