chore(types): Type-clean colang/ (111 errors) #1381
Conversation
|
Tagging @cparisien, @Pouyanpi , @trebedea for review |
tgasser-nv
changed the title
tgasser-nv
changed the base branch from
chore/type-clean-guardrails
to
develop
Documentation preview |
tgasser-nv
changed the base branch from
develop
to
chore/type-clean-guardrails
tgasser-nv
changed the base branch from
chore/type-clean-guardrails
to
develop
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
tgasser-nv
changed the base branch from
develop
to
chore/type-clean-guardrails
tgasser-nv
changed the base branch from
chore/type-clean-guardrails
to
develop
tgasser-nv
changed the base branch from
develop
to
chore/type-clean-guardrails
tgasser-nv
changed the base branch from
chore/type-clean-guardrails
to
develop
tgasser-nv
changed the base branch from
develop
to
chore/type-clean-guardrails
tgasser-nv
changed the base branch from
chore/type-clean-guardrails
to
develop
tgasser-nv
force-pushed
the
chore/type-clean-colang
branch
from
f5e281f to
489b2a3
Compare
tgasser-nv
changed the base branch from
develop
to
chore/type-clean-guardrails
tgasser-nv
changed the base branch from
chore/type-clean-guardrails
to
develop
tgasser-nv
force-pushed
the
chore/type-clean-colang
branch
from
b4a4f85 to
f507ea0
Compare
|
Converting to draft while I rebase on the latest changes to |
tgasser-nv
force-pushed
the
chore/type-clean-colang
branch
from
f507ea0 to
76caade
Compare
|
Refreshed this PR based on the latest |
|
@tgasser-nv I created a dummy commit and can see around 63 pyright errors when pre-commits are run. Would you please have a look? |
tgasser-nv
force-pushed
the
chore/type-clean-colang
branch
from
647cc32 to
dbeba8b
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR systematically eliminates 111 Pyright type-checking errors across the nemoguardrails/colang/ directory by adding defensive null-checks, type guards, and explicit type annotations. The changes prevent potential runtime TypeError, AttributeError, and KeyError exceptions by validating that variables are not None before dereferencing, providing default values for optional parameters, and wrapping non-dict action results into dictionaries where expected. The modifications span both Colang v1.0 and v2.x parsers, runtimes, and AST processors, following a defensive programming pattern that silently skips operations when values are missing rather than failing fast. This aligns with the broader Guardrails architecture where robustness in parsing and flow execution is prioritized over strict validation.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
pyproject.toml |
5/5 | Adds nemoguardrails/colang/** to Pyright include list to enable type-checking for the module |
nemoguardrails/logging/processing_log.py |
5/5 | Adds type annotation to processing_log_var context variable for improved static analysis |
nemoguardrails/actions/llm/generation.py |
5/5 | Removes explicit -> None return type hint to fix type-checker complaints about implicit returns |
nemoguardrails/colang/v2_x/lang/colang_ast.py |
5/5 | Adds __hash__ method to Element class to satisfy Python's hashable protocol |
nemoguardrails/colang/v2_x/runtime/serialization.py |
4/5 | Adds type guard after deserialization to ensure State instance before accessing attributes |
nemoguardrails/colang/v1_0/runtime/sliding.py |
5/5 | Wraps imports in TYPE_CHECKING block to resolve circular dependencies |
nemoguardrails/colang/v2_x/runtime/flows.py |
5/5 | Uses TYPE_CHECKING guard for RailsConfig import to prevent circular dependency |
nemoguardrails/colang/v1_0/lang/comd_parser.py |
5/5 | Adds null-check for sym variable before processing symbol mappings |
nemoguardrails/colang/runtime.py |
4/5 | Adds null-check for config.imported_paths and uses getattr() for conditional method registration |
nemoguardrails/colang/v2_x/lang/parser.py |
4/5 | Corrects return type annotation and adds null-check for import_el.package |
nemoguardrails/colang/v2_x/lang/transformer.py |
5/5 | Renames variable to avoid shadowing and broadens return type to Any |
nemoguardrails/colang/v2_x/runtime/eval.py |
5/5 | Fixes lambda closure bugs and adds type-cast for SimplifyFormatter().format() |
nemoguardrails/colang/v1_0/lang/colang_parser.py |
3/5 | Adds extensive null-checks and type annotations throughout parser logic |
nemoguardrails/colang/v1_0/lang/utils.py |
3/5 | Adds defensive null-checks for string variables and introduces unused multiline_indentation |
nemoguardrails/colang/v1_0/runtime/flows.py |
3.5/5 | Adds null-checks for slide() return values but has return-type inconsistency in _slide_with_subflows() |
nemoguardrails/colang/v1_0/runtime/runtime.py |
4/5 | Adds guards for None values and wraps non-dict action results silently |
nemoguardrails/colang/v2_x/lang/expansion.py |
4/5 | Adds compound type-safety guards for element._source.line and spec element validation |
nemoguardrails/colang/v2_x/lang/utils.py |
5/5 | Adds critical type guard to prevent TypeError when calling asdict() on dataclass classes |
nemoguardrails/colang/v2_x/runtime/statemachine.py |
4/5 | Comprehensive null-checks across state machine runtime with repeated defensive patterns |
nemoguardrails/colang/v1_0/lang/coyml_parser.py |
4/5 | Adds defensive checks for dictionary keys and converts jump offsets to strings |
nemoguardrails/colang/v2_x/runtime/runtime.py |
4/5 | Adds conditional checks for langchain attributes and filters Event objects from history |
Confidence score: 3.5/5
- This PR improves robustness but introduces several silent failure modes where exceptions or missing values are ignored rather than raising errors, which could mask bugs
- Score reflects that while the type-safety improvements reduce immediate crash risk, several changes have medium risk:
_slide_with_subflows()always returnsNonedespite declaringOptional[int]return type, unusedmultiline_indentationvariable in utils.py, duplicateRailsConfigimport in flows.py, silent wrapping of action results without logging, and repeated defensive patterns that could benefit from refactoring - Pay close attention to
nemoguardrails/colang/v1_0/runtime/flows.py(return type mismatch),nemoguardrails/colang/v1_0/lang/utils.py(dead code), andnemoguardrails/colang/v1_0/runtime/runtime.py(silent error suppression)
Additional Comments (1)
-
nemoguardrails/colang/v1_0/lang/coyml_parser.py, line 437-450 (link)style: Converting
_next_elsefrom int to str and then back to int (line 450) is unnecessary. Keep it as int throughout or document why string representation is required by the runtime.
21 files reviewed, 19 comments
| if not isinstance(state, State): | ||
| raise ValueError("Decoded object is not a State instance") |
There was a problem hiding this comment.
logic: Check that decode_from_dict returns a dict with expected structure before casting to State, since decode_from_dict can return primitives or other types. Should we verify the decoded data has the expected structure (e.g., flow_states attribute) before the isinstance check?
| stripped_tokens = get_stripped_tokens(split_max(flow_name, "=", 1)) | ||
| return_vars, flow_name = stripped_tokens[0], stripped_tokens[1] |
There was a problem hiding this comment.
logic: When split_max returns only 1 token, stripped_tokens[1] will cause IndexError
| current_comment = None | ||
| multiline_string = False | ||
| current_string = None | ||
| multiline_indentation = 0 |
There was a problem hiding this comment.
logic: multiline_indentation is initialized but never written to after this point. The value set on line 120 is never used because it's only set inside the multiline_string block that never reads this variable.
| # Ensure element is valid for SpecOp | ||
| if isinstance(element, (dict, Spec)): | ||
| spec_element: Union[dict, Spec] = element | ||
| else: | ||
| spec_element = {} |
There was a problem hiding this comment.
style: Wrapping with an empty dict {} as fallback could hide programming errors. If element is not a valid dict or Spec, the error might surface later in unexpected ways rather than failing fast here. Consider logging a warning or using a more defensive default. Should invalid elements be logged or raise an error instead of silently defaulting to {}?
| if e.args[0] and current_flow_config is not None: | ||
| raise ColangSyntaxError( | ||
| e.args[0] + f" in flow `{flow_config.id}` ({flow_config.source_file})" | ||
| e.args[0] | ||
| + f" in flow `{current_flow_config.id}` ({current_flow_config.source_file})" | ||
| ) | ||
| else: | ||
| raise ColangSyntaxError() from e |
There was a problem hiding this comment.
style: Consider checking e.args length before accessing e.args[0] to avoid IndexError
| if isinstance(member, dict) and "name" in member: | ||
| event_name = member["name"] | ||
| elif hasattr(member, "name"): | ||
| event_name = member.name | ||
| else: | ||
| # Fall back to string representation | ||
| event_name = str(member) |
There was a problem hiding this comment.
style: This branching logic is duplicated at lines 2293-2295. Consider extracting a helper function get_member_name(member)
| flow_event_name = element_spec.members[0]["name"] | ||
| flow_event: InternalEvent = temp_flow_state.get_event(flow_event_name, {}) | ||
| del flow_event.arguments["source_flow_instance_uid"] | ||
| del flow_event.arguments["flow_instance_uid"] | ||
| return flow_event.name | ||
| if flow_event_name is not None: |
There was a problem hiding this comment.
logic: Dictionary access element_spec.members[0]["name"] will raise KeyError if "name" key is missing. Previous code at lines 2213-2219 handles both dict and object cases - this should too
| "Local action finished with an exception!", | ||
| exc_info=True, | ||
| ) | ||
| result = None # Set a default value for failed actions |
There was a problem hiding this comment.
style: Setting result to None silently discards exception info - consider logging the failed action details or wrapping in ActionResult
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Note: This review covers only the changes made in nemoguardrails/colang/v1_0/runtime/flows.py since the last review, not the entire PR.
This update addresses a circular import issue and adds defensive null-checking in the Colang v1.0 runtime flows module. The developer introduced from __future__ import annotations to enable deferred type evaluation and moved the RailsConfig import into a TYPE_CHECKING guard, breaking a circular dependency between the flows module and the config module. Additionally, a guard was added to validate that event.get("data") returns a non-None dictionary before calling context.update(), preventing TypeError when malformed events are processed. These changes align with the broader PR effort to eliminate 111 type errors across the colang codebase while preserving runtime behavior.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
nemoguardrails/colang/v1_0/runtime/flows.py |
5/5 | Resolved circular import by deferring RailsConfig import to type-checking time and added null-safe guard for event data before context update |
Confidence score: 5/5
- This change is safe to merge with minimal risk, addressing type-checking and circular-import issues without functional regression.
- Score reflects defensive programming that prevents runtime crashes without altering control flow, plus comprehensive test coverage (1552 tests passed, pyright reports zero errors).
- No files require special attention; the changes are localized, well-tested, and follow established Python type-safety patterns.
1 file reviewed, no comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The latest update addresses the previously flagged duplicate RailsConfig import in nemoguardrails/colang/v1_0/lang/comd_parser.py by removing the runtime import on line29, keeping only the conditional import within the TYPE_CHECKING block. This change improves import hygiene and resolves a static-analysis warning without altering any runtime behavior. The modification integrates cleanly with the broader PR's goal of eliminating type-checking errors—the duplicate import was a minor linting issue that would have been flagged by tools like pyright or isort, and removing it ensures the codebase passes pre-commit hooks.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| nemoguardrails/colang/v1_0/lang/comd_parser.py | 5/5 | Removed redundant runtime import of RailsConfig (line 29), keeping only the TYPE_CHECKING conditional import for cleaner type-hint support |
Confidence score: 5/5
- This change is safe to merge with no risk—it is a pure import cleanup with zero functional impact.
- Score reflects a trivial non-breaking change that directly addresses a previous review comment; the duplicate import served no purpose at runtime and only existed for type hints (already covered by the
TYPE_CHECKINGblock). - No files require special attention—the single-line removal is straightforward and isolated.
Additional Comments (1)
-
nemoguardrails/colang/v1_0/lang/comd_parser.py, line 373-374 (link)logic: When
split_max(sym, ':', 1)returns only 1 token (no colon),object_nameis assignedsymitself, which may include the prefix. Consider usingobject_name = split_max(sym, ':', 1)[1] if ':' in sym else symor verifying thatsymhas been prefixed by_get_typed_symbol_nameon line 368.
2 files reviewed, 1 comment
3 participants
Description
Summary
This report details the type-safety enhancements and bug fixes introduced in the pull request. The changes primarily focus on preventing runtime
TypeError,AttributeError, andKeyErrorexceptions by handling potentiallyNonevalues and ensuring variables conform to their expected types.Medium Risk
Changes in this category add new logical branches (e.g.,
if obj is not None) or make assumptions about default behavior. While they significantly improve robustness, they alter execution flow and warrant careful review.1. Guarding Against
NoneValues Before AccessThis is the most common fix, adding checks to ensure an object is not
Nonebefore its attributes or keys are accessed. This preventsAttributeErrorandTypeError.Files:
nemoguardrails/colang/v1_0/lang/colang_parser.py, Line 340nemoguardrails/colang/v1_0/lang/colang_parser.py, Line 962nemoguardrails/colang/v1_0/lang/colang_parser.py, Line 1014nemoguardrails/colang/v1_0/lang/colang_parser.py, Line 1370nemoguardrails/colang/v1_0/lang/comd_parser.py, Line 365nemoguardrails/colang/v2_x/runtime/runtime.py, Line 390Original Error: Potential
TypeError: 'NoneType' object is not subscriptableorAttributeError: 'NoneType' object has no attribute '...'.Example Fix (
colang_parser.py, Line 962):Explanation:
self.current_elementandyaml_valueare notNonebefore attempting to iterate over keys and perform assignments.None, the operation should be silently skipped.ValueErrorifself.current_elementisNone, enforcing that it must be initialized before this function is called. However, skipping is a more defensive and robust approach in a complex parser.2. Providing Default Values for Optional Variables
This pattern handles cases where a variable might be
Noneby substituting a sensible default (e.g., an empty list, empty string, or0).Files:
nemoguardrails/colang/runtime.py, Line 37nemoguardrails/colang/v1_0/lang/colang_parser.py, Line 315nemoguardrails/colang/v1_0/lang/coyml_parser.py, Line 424nemoguardrails/colang/v1_0/runtime/flows.py, Line 471Original Error: Potential
TypeErrorwhen a function expects an iterable or number but receivesNone.Example Fix (
coyml_parser.py, Line 424):Explanation:
[]as a default if the"then"or"else"keys are missing fromif_elementor are not lists. This ensures_extract_elementsalways receives an iterable.3. Explicit Type Casting and Result Wrapping
This involves explicitly converting a variable to a required type (e.g.,
str(),int()) or wrapping a return value to conform to an expected structure (e.g., wrapping a value in a dictionary).Files:
nemoguardrails/colang/v1_0/runtime/flows.py, Line 263nemoguardrails/colang/v1_0/runtime/runtime.py, Line 798nemoguardrails/colang/v1_0/lang/coyml_parser.py, Line 434Original Error: Potential
TypeErrorif a function receives an object of an unexpected type.Example Fix (
runtime.py, Line 798):Explanation:
_get_action_respis expected to return a dictionary. This fix wraps any non-dictionary return value into adictwith a"value"key.Low Risk
These changes involve adding or correcting type hints and performing minor refactors that do not alter the program's logic. They improve code clarity and static analysis without any risk of functional regression.
1. Addition of Standard Type Hints
This involves adding explicit type annotations to variables and function signatures, which helps static analysis tools catch errors.
Files:
nemoguardrails/colang/v1_0/lang/colang_parser.py, Line 129nemoguardrails/colang/v1_0/runtime/flows.py, Line 95nemoguardrails/logging/processing_log.py, Line 25Original Error: Lack of type information, making static analysis less effective.
Example Fix (
colang_parser.py, Line 129):Explanation:
self.current_elementis now explicitly typed as being either a dictionary orNone. This allows the type checker (mypy) to validate its usage throughout the class.2. Assertions to Guide the Type Checker
This pattern uses
assertstatements to inform the type checker that a variable is notNonewithin a specific code block, narrowing its type.File:
nemoguardrails/colang/v1_0/lang/colang_parser.py, Line 1836Original Error: The type checker would incorrectly flag usage of
snippetas a potentialTypeErrorbecause it was initialized toNone.Example Fix (
colang_parser.py, Line 1836):Explanation:
assert snippet is not Nonestatement guarantees to the static analyzer that from this point forward,snippetcannot beNone, thus making the accesssnippet["lines"]type-safe.snippethas been assigned a dictionary by the time this line is reached.if snippet is not None:block could be used, but since the logic implies it can't beNonehere, an assertion is more appropriate to catch logic errors during development.Test Plan
Type-checking
$ poetry run pre-commit run --all-files check yaml...............................................................Passed fix end of files.........................................................Passed trim trailing whitespace.................................................Passed isort (python)...........................................................Passed black....................................................................Passed Insert license in comments...............................................Passed pyright..................................................................PassedUnit-tests
Local CLI check
Checklist