Skip to content

pre release: fix main.yml#3263

Merged
jmcouffin merged 28 commits intomasterfrom
develop
Apr 5, 2026
Merged

pre release: fix main.yml#3263
jmcouffin merged 28 commits intomasterfrom
develop

Conversation

@jmcouffin
Copy link
Copy Markdown
Contributor

No description provided.

thanhtranarch and others added 28 commits March 31, 2026 21:51
Added T3LabLite extension with details including author, description, and URLs.
…t window

Fixes #1729

The pyRevit output window hosts a WebBrowser control inside a WPF
MetroWindow via WindowsFormsHost. Revit's message loop intercepts
Ctrl+C via IOleInPlaceActiveObject.TranslateAccelerator before WPF,
WinForms, IMessageFilter, or JavaScript events can see the keystroke.

This adds a Win32 low-level keyboard hook (WH_KEYBOARD_LL) that
intercepts Ctrl+C and Ctrl+A at the OS level, before Revit's
accelerator table processes them. The hook only activates when the
ScriptConsole is the active window with renderer focus, and forwards
the keystrokes to HtmlDocument.ExecCommand for native browser copy.

The hook is installed on window creation and disposed on close.
 Add Ctrl+C and Ctrl+A keyboard shortcuts to ScriptConsole output window
Use core.set_option() instead of set_thirdparty_ext_root_dirs() when registering
extension paths. This avoids the existence check in set_thirdparty_ext_root_dirs()
which would fail for temporarily-offline network shares, defeating the purpose of
reading the raw list.

Fixes the issue identified in review: set_thirdparty_ext_root_dirs() raises
PyRevitException for any path that doesn't currently exist on disk.
…egistered

- Move CONSTS import outside try block so it's available for both the
  read and write paths regardless of whether an exception occurs.
- Replace set_thirdparty_ext_root_dirs() with core.set_option() to avoid
  the existence-check rejection of temporarily-offline network shares when
  writing back the updated directory list.
- Capture the read exception and log it at debug level before falling back
  to the helper, replacing the silent bare 'except Exception' swallow.
…ause-by-Copilot-Auto-Fix

Fix Regressions cause by Copilot Auto Fix
SeedEnvironmentDictionary() was called before _pyRevitRoot was resolved,
so EnvDictionarySeeder.Seed() received an empty string for pyRevitRoot.
Both ReadPyRevitVersion() and ReadIPYVersion() returned "Unknown" because
the repo root path was empty, skipping the version file and IronPython.dll
lookup entirely.

Fix: eagerly resolve _pyRevitRoot from _binDir inside InitializeScriptExecutor(),
which runs immediately before SeedEnvironmentDictionary() in LoadSession().
The executing assembly lives at {pyRevitRoot}/bin/, so FindPyRevitRoot(_binDir)
walks up one level and finds pyrevitlib/. If it fails (non-standard layout),
_pyRevitRoot stays null and BuildSearchPaths() retries later with the extension
directory as an additional hint.
Log whether pyRevit root resolution succeeded or fell back to null,
making it easier to diagnose version-display issues in the output window.
…3260)

Add PyRevitRootResolutionTests to verify that FindPyRevitRoot correctly
resolves the repo root from a bin/ subdirectory (via pyRevitfile marker
or pyrevitlib/ directory), and that ReadPyRevitVersion/ReadIPYVersion
return actual values (not 'Unknown') when given a valid root.

End-to-end tests replicate the exact bug scenario: without early
resolution of _pyRevitRoot in InitializeScriptExecutor(), the
SeedEnvironmentDictionary() call receives an empty string, causing
both version reads to short-circuit to 'Unknown'.

Changes:
- SessionManagerService.FindPyRevitRoot: private → internal (testable)
- EnvDictionarySeeder.ReadPyRevitVersion: private → internal (testable)
- New PyRevitRootResolutionTests.cs with 11 test cases
- Updated .csproj to include the new test file
…-output

fix: resolve pyRevit/IPY version showing Unknown in output window
ReadIPYVersion() had three bugs:
1. Searched wrong paths (bin/IPY342/) instead of the actual layout
   (bin/{netcore|netfx}/engines/{IPY342|IPY2712PR}/)
2. Only checked for "IronPython.dll" — missing the IPY2712PR variant
   named "pyRevitLabs.IronPython.dll"
3. Checked the executing assembly directory last instead of first,
   reporting the wrong engine version when multiple engines are present

Fix: check the active engine directory first (Assembly.GetExecutingAssembly
sits alongside the active IronPython DLL), use correct bin/ layout for
fallback paths, check both DLL name variants, and add Trace.WriteLine
to the catch block for diagnosability.
…nknown

fix: resolve IronPython version showing Unknown in output window
Bugfixes for sectionbox navigator
Update extensions.json - Add T3LabLite extension details
@jmcouffin jmcouffin merged commit 0cc6d74 into master Apr 5, 2026
1 check passed
Copy link
Copy Markdown
Contributor

@devloai devloai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary:

  • Adds a push trigger to main.yml so that signing secrets (unavailable for fork PRs) are available on the resulting push after merge
  • Adds a pre-signing step to validate all Azure Trusted Signing secrets are present
  • Fixes EnvDictionarySeeder.ReadIPYVersion to prioritize the active engine directory and support both IronPython DLL name variants
  • Adds regression tests for the pyRevit root resolution ordering bug (PR #3260)
  • Section Box Navigator: switches from Level.Elevation to Level.ProjectElevation and introduces update_grids_and_levels() helper
  • Extensions manager: reads raw config list to preserve offline network paths

Review Summary:

The core intent of the main.yml fix is sound — fork PRs don't have signing secrets, so a push trigger ensures the merge commit is signed. However, there are two workflow correctness issues: the new push trigger will fire a full duplicate pipeline run on every same-repo merge (once for pull_request: closed, once for push), and the actual signing steps lack the WipRun/ReleaseRun guard that the new secrets-validation step has, meaning every routine push to develop/master will consume Azure signing quota.

The Python changes are clean and follow IronPython patterns. The EnvDictionarySeeder improvements and new regression tests are well-structured. One fragile pattern in the Section Box Navigator (update_grids_and_levels capturing the module-level doc global in an event-driven context) and a misleading hasattr guard are flagged.

Suggestions

  • Add a dedup guard to the build job (e.g., skip push events when a pull_request event for the same commit already ran) to avoid double builds on internal merges. Apply
  • Apply the WipRun/ReleaseRun/workflow_dispatch condition to both signing steps so routine push commits don't consume Azure signing quota. Apply

Apply all quick fixes (3 quick fixes)

if: >
github.repository == env.MainRepo &&
(github.event_name != 'pull_request' ||
github.event.pull_request.head.repo.full_name == github.repository)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signing steps for DLLs/EXEs and installers are gated only on the repo identity and PR-from-fork check, but not on WipRun/ReleaseRun. This means every direct push to develop or master (even routine commits without a release intent) will attempt to sign artifacts — consuming Azure signing quota unnecessarily.

The upstream "Require Trusted Signing secrets" step already has the WipRun/ReleaseRun/push/workflow_dispatch guard, but the actual signing steps do not. Consider aligning with the same condition (minus the bare push clause, since signing every push is what we're trying to avoid):

        if: >
          github.repository == env.MainRepo &&
          (github.event_name != 'pull_request' ||
           github.event.pull_request.head.repo.full_name == github.repository) &&
          (env.WipRun == 'true' || env.ReleaseRun == 'true' ||
           github.event_name == 'workflow_dispatch')

Same issue applies to the installer signing step at line 190.

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.

- 'release/**'
- 'site-packages/**'
# After merges (especially from forks), secrets are available on push but not on pull_request.
push:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a push trigger on develop/master alongside the existing pull_request trigger creates duplicate CI runs for internal PRs (non-forks): the PR merge generates both a pull_request (type: closed) event and a push event, causing the entire build pipeline to run twice.

The build job condition github.event_name != 'pull_request' || github.event.action != 'closed' || github.event.pull_request.merged already passes for the push event unconditionally — so there's no filtering that avoids the duplication.

A common mitigation is to skip the push event when it was triggered by a PR merge commit, e.g.:

jobs:
  build:
    if: >
      github.event_name == 'workflow_dispatch' ||
      (github.event_name == 'push' && github.event.head_commit.author.name != 'GitHub Actions') ||
      (github.event_name == 'pull_request' && (github.event.action != 'closed' || github.event.pull_request.merged))

Or alternatively, only use push for fork-originating commits (detect via github.event.head_commit not having a pull_request origin) and skip when a matching pull_request event already handled it.

actions

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.


def update_grids_and_levels(self):
"""Updates grids and levels"""
self.all_levels = get_all_levels(doc, self.chkIncludeLinks.IsChecked)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update_grids_and_levels() captures the module-level doc variable at call time. When this method is invoked from the on_view_or_doc_changed event handler after initialize_globals() has refreshed doc to the new document, this is fine. But if it's ever called from a Dispatcher-posted callback on a different thread before the module-level doc is updated (e.g., race between the event handler and the Dispatcher), the stale doc reference will be used.

More concretely: on_view_or_doc_changed first calls initialize_globals() (updates doc), then posts update_grids_and_levels via Dispatcher.Invoke. Since Invoke is synchronous, this is safe today. However, the pattern is fragile — update_grids_and_levels should explicitly receive the document it operates on rather than capturing the global:

def update_grids_and_levels(self, target_doc=None):
    """Updates grids and levels"""
    target_doc = target_doc or doc
    self.all_levels = get_all_levels(target_doc, self.chkIncludeLinks.IsChecked)
    self.all_grids = get_all_grids(target_doc, self.chkIncludeLinks.IsChecked)

This matches the pyRevit IronPython guideline: avoid relying on module-level document references in callbacks.

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.

try:
# I assume the event handler is sometimes faster than the init, causing a initial error message
# therefore safeguard with:
if not hasattr(self, "current_view"):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hasattr(self, "current_view") guard is unnecessary and misleading. The event handler on_view_or_doc_changed already gates on if sb_form (line 177), and sb_form is only assigned after SectionBoxNavigatorForm.__init__() fully completes (line 1984). So update_info() can never be invoked during __init__ via the event pathway.

Furthermore, self.current_view is unconditionally set at line 204 — before update_grids_and_levels() at line 208 — so it is always present by the time the form is shown.

A more meaningful early-exit guard would check the attributes that are actually None-initialized during construction:

def update_info(self):
    """Update the information display."""
    try:
        if self.all_levels is None or self.all_grids is None:
            return  # Not yet fully initialized
        last_view = self.current_view.Id

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 5, 2026

📦 New public release are available for 6.3.0.26095+0830

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants