fix: isolate per-extension failures so one bad extension can't drop the rest#2951
Open
PascalThuet wants to merge 3 commits into
Open
fix: isolate per-extension failures so one bad extension can't drop the rest#2951PascalThuet wants to merge 3 commits into
PascalThuet wants to merge 3 commits into
Conversation
…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
Contributor
There was a problem hiding this comment.
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/exceptand 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
Contributor
Author
|
Updated on behalf of @PascalThuet by Codex (GPT-5). Commit: 5cdbc5f What changed:
Validation run locally:
GitHub now reports the PR as mergeable. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
ExtensionManager.register_enabled_extensions_for_agentiterates over enabled extensions with no per-extension error isolation. If registering one extension raised (e.g. anOSErrorwriting 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-efforttry/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 switchand (since #2949 / #2886)integration installandintegration upgrade.Fix
Wrap the per-extension loop body in
try/except: on failure, warn (naming the failing extension) andcontinueto 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_restinstalls 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