Skip to content

fix: isolate per-extension failures so one bad extension can't drop the rest#2951

Open
PascalThuet wants to merge 3 commits into
github:mainfrom
PascalThuet:fix/2950-extension-registration-resilience
Open

fix: isolate per-extension failures so one bad extension can't drop the rest#2951
PascalThuet wants to merge 3 commits into
github:mainfrom
PascalThuet:fix/2950-extension-registration-resilience

Conversation

@PascalThuet

Copy link
Copy Markdown
Contributor

What

ExtensionManager.register_enabled_extensions_for_agent iterates over enabled extensions with no per-extension error isolation. If registering one extension raised (e.g. an OSError writing a command file), the loop aborted and the exception propagated, so every subsequent enabled extension was silently skipped. The callers wrap the whole call in a single best-effort try/except, so the wholesale abort surfaced as one warning while the command still exited 0 — leaving the agent with only a prefix of its extensions registered.

This path is reached by integration switch and (since #2949 / #2886) integration install and integration upgrade.

Fix

Wrap the per-extension loop body in try/except: on failure, warn (naming the failing extension) and continue to the rest. The caller-level best-effort catch remains as a backstop.

Note the loop already guards the non-raising degraded cases — a manifest that fails to load is skipped (get_extension(...) is None → continue) and an empty registration result clears stale state — so this only adds isolation for genuine exceptions mid-loop.

Test

test_one_failing_extension_does_not_abort_the_rest installs two enabled extensions, forces the first-iterated one's registration to raise, and asserts the later extension still registers and the call does not propagate. Confirmed it fails without the fix.

Full suite: 3726 passed, 45 skipped locally.

Closes #2950

…r_agent

The per-extension loop had no error isolation: if registering one enabled
extension raised (e.g. an OSError writing a command file), the loop aborted and
the exception propagated, so every subsequent enabled extension was silently
skipped. Callers wrap the whole call in a single best-effort try/except, so the
wholesale abort surfaced as one warning while the command still exited 0 —
leaving the agent with only a prefix of its extensions.

Wrap the per-extension body in try/except: warn (naming the extension) and
continue, so one bad extension can no longer drop the others. Add a regression
test that forces the first-iterated extension to raise and asserts the rest
still register.

Closes github#2950

Copilot AI left a comment

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.

Pull request overview

This PR improves robustness of extension command/skill registration by isolating failures per extension inside ExtensionManager.register_enabled_extensions_for_agent, ensuring one broken extension can’t prevent subsequent enabled extensions from registering.

Changes:

  • Wrap per-extension registration work in try/except and emit a targeted warning on failure, then continue.
  • Add a regression test proving that a failing extension does not abort registration of later extensions.
Show a summary per file
File Description
src/specify_cli/extensions.py Adds per-extension exception isolation during enabled-extension registration, warning and continuing on failures.
tests/test_extension_skills.py Adds regression coverage ensuring one failing extension doesn’t prevent later extensions from registering.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

Comment thread src/specify_cli/extensions.py Outdated
@PascalThuet

Copy link
Copy Markdown
Contributor Author

Updated on behalf of @PascalThuet by Codex (GPT-5).

Commit: 5cdbc5f

What changed:

  • Addressed Copilot review 4506765356 by isolating _register_extension_skills() failures from successful command registration inside register_enabled_extensions_for_agent.
  • Successful command registrations are now still persisted to registered_commands even when companion skill generation raises, preserving later cleanup/unregister behavior.
  • Added regression coverage for the command-registration-succeeds / skill-registration-fails case.
  • Merged current origin/main into the PR branch. The merge completed without manual conflicts.

Validation run locally:

  • uv run pytest tests/test_extension_skills.py -q -> 53 passed
  • uvx ruff check src/specify_cli/extensions.py tests/test_extension_skills.py -> passed
  • git diff --check -> passed

GitHub now reports the PR as mergeable. gh pr checks currently reports no checks on the branch.

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.

register_enabled_extensions_for_agent has no per-extension error isolation: one failing extension silently drops the rest

3 participants