pyRevit 6.0.0 Release

[jmcouffin]

No description provided.

[devloai]

PR Summary:

This is a major release (pyRevit 6.0.0) merging develop branch into master. Key changes include:

• New C# session manager with Roslyn build strategy as an alternative to Python-based loader
• Enhanced runtime support with custom variables in ScriptRuntimeConfigs for both CPython and IronPython engines
• Revit 2027 compatibility updates (new runtime assemblies, API compatibility fixes)
• Improved installer PATH handling with admin elevation support
• New translation helper scripts for bundle localization workflows
• Enhanced UI with dynamic settings window library and improved localization support

Review Summary:

Reviewed 500+ changed files focusing on critical infrastructure: C# runtime engines, Python session management, security-sensitive installer scripts, and new translation tooling. Applied repository standards requiring IronPython 2.7.12 compatibility and PEP8 conventions.

Key findings:

• One high-severity security inconsistency between CPython and IronPython engines regarding reserved builtin protection
• Two medium-severity bugs in exception handling (silent swallowing in session loader, chardet encoding validation)
• Two low-severity code smell improvements for cleaner exception handling patterns

The PR shows generally solid engineering with proper defensive patterns, XML escaping, and validation atomicity. Main concerns are security consistency and observability in critical session loading paths.

Follow-up suggestions:

• @devloai fix the identified security issue in CPythonEngine.cs
• @devloai improve exception logging in session manager

[devloai]

Security: User-provided variables can override reserved built-ins in CPython

The CPython engine lacks the reserved builtin name protection that IronPython has (lines 251-273 in IronPythonEngine.cs). User-provided variables in ScriptRuntimeConfigs.Variables could potentially overwrite critical pyRevit built-ins like __revit__, __commanddata__, etc.

This is a security risk if user configuration can inject malicious variables.

Recommendation:
Apply the same reservedBuiltinNames HashSet check here as in IronPythonEngine.cs:

if (runtime.ScriptRuntimeConfigs?.Variables != null) {
    var reservedBuiltinNames = new HashSet<string> {
        "__execid__", "__timestamp__", "__cachedengine__", 
        "__cachedengineid__", "__scriptruntime__", "__revit__",
        "__commanddata__", "__elements__", "__uibutton__",
        "__commandpath__", "__configcommandpath__", "__commandname__",
        "__commandbundle__", "__commandextension__", "__commanduniqueid__",
        "__commandcontrolid__", "__forceddebugmode__", "__shiftclick__",
        "__result__", "__eventsender__", "__eventargs__"
    };
    
    foreach (var variable in runtime.ScriptRuntimeConfigs.Variables) {
        if (reservedBuiltinNames.Contains(variable.Key))
            continue;
        SetVariable(builtins, variable.Key, variable.Value);
    }
}

⚡ Apply quick fix

actions

Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).

Feedback: Rate this comment to help me improve future code reviews:

• 👍 Good - Helpful and accurate
• 👎 Poor - Wrong, unclear, or unhelpful
• Skip if you don't have any strong opinions either way.

[devloai]

Bug: Silent exception swallowing hides assembly loading failures

The bare except Exception: on line 279 silently swallows all exceptions when trying to access assembly name/location. This can hide legitimate errors like security exceptions, IO exceptions, or reflection errors that should be logged for debugging.

According to the review checklist: "Empty catch blocks in C# for UI operations may be acceptable if failure doesn't impact core functionality" but this is core session loading logic, not UI.

Recommendation:
Log the exception at debug level to aid troubleshooting:

except Exception as ex:
    # Some assemblies might not have accessible location/name
    mlogger.debug('Failed to check assembly %s: %s', 
                  assembly, ex)
    continue

⚡ Apply quick fix

actions

Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).

Feedback: Rate this comment to help me improve future code reviews:

• 👍 Good - Helpful and accurate
• 👎 Poor - Wrong, unclear, or unhelpful
• Skip if you don't have any strong opinions either way.

[devloai]

Code Smell: Duplicate exception swallowing pattern

Similar to the previous issue, another silent exception handler on line 333 in _register_loaded_pyrevit_assemblies(). This makes debugging assembly registration failures very difficult.

Recommendation:

except Exception as ex:
    mlogger.debug('Failed to check assembly: %s', ex)
    continue

⚡ Apply quick fix

actions

Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).

Feedback: Rate this comment to help me improve future code reviews:

• 👍 Good - Helpful and accurate
• 👎 Poor - Wrong, unclear, or unhelpful
• Skip if you don't have any strong opinions either way.

[devloai]

Code Smell: Repetitive exception handling for resource lookups

The pattern of catching wpf.ResourceReferenceKeyNotFoundException is repeated multiple times throughout the file (lines 907-910, 1071-1081, 1104-1114). While this works correctly, using exceptions for control flow when a resource key is missing is not ideal for performance or code clarity.

Minor concern: Exception-driven flow control for missing resources in production code.

Recommendation:
Consider creating a helper method to avoid repetition:

def try_get_locale_string(self, key, fallback=None):
    """Try to get localized string, return fallback if not found."""
    try:
        return self.get_locale_string(key)
    except wpf.ResourceReferenceKeyNotFoundException:
        return fallback

# Then use:
self.select_b.Content = self.try_get_locale_string(
    "SelectFromList.Select.Button", "Select"
)

This avoids the repetitive try-except pattern and makes the code cleaner.

⚡ Apply quick fix

actions

Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).

Feedback: Rate this comment to help me improve future code reviews:

• 👍 Good - Helpful and accurate
• 👎 Poor - Wrong, unclear, or unhelpful
• Skip if you don't have any strong opinions either way.

[devloai]

Bug: Chardet encoding detection can fail silently

Line 34: enc = chardet.detect(raw.read())["encoding"]

Issues:

1. chardet.detect() can return {"encoding": None, "confidence": 0.0} if it fails to detect encoding
2. Accessing the "encoding" key directly with bracket notation will succeed even if the value is None
3. Line 39 will then fail with TypeError: 'encoding' must be str, not None when opening with encoding=enc

Per review checklist (lines 146-152):

• "When using libraries that return nullable results, validate return values before accessing"
• "Avoid accessing dictionary keys with bracket notation on potentially None results; use .get() with defaults instead"

Recommendation:

with open(csv_path, "rb") as raw:
    detection = chardet.detect(raw.read())
    enc = detection.get("encoding")
    confidence = detection.get("confidence", 0.0)
    
    if not enc or confidence < 0.7:
        print(f"Low confidence ({confidence}) for encoding detection, using utf-8")
        enc = "utf-8"
    else:
        print(f"Detected encoding: {enc} (confidence: {confidence})")

with open(csv_path, encoding=enc) as f:
    # ...

⚡ Apply quick fix

actions

Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).

Feedback: Rate this comment to help me improve future code reviews:

• 👍 Good - Helpful and accurate
• 👎 Poor - Wrong, unclear, or unhelpful
• Skip if you don't have any strong opinions either way.