fix: isolate per-extension failures so one bad extension can't drop the rest#2951
fix: isolate per-extension failures so one bad extension can't drop the rest#2951PascalThuet wants to merge 3 commits into
Conversation
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
|
Updated on behalf of @PascalThuet by Codex (GPT-5). Commit: 5cdbc5f What changed:
Validation run locally:
GitHub now reports the PR as mergeable. |
| continuing=( | ||
| "Continuing with command registration for this " | ||
| "extension and the remaining extensions." | ||
| ), |
| assert "register extension skills for extension 'skill-fail'" in captured.out | ||
| assert "Continuing with command registration" in captured.out | ||
|
|
|
Please address Copilot feedback and rebase on upstream/main |
…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
5cdbc5f to
c899cd3
Compare
|
Updated on behalf of @PascalThuet by Codex (GPT-5). Commit: c899cd3 What changed:
Validation run locally:
|
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