diff --git a/.pql/changelog/ticket_history/2026-06.sql b/.pql/changelog/ticket_history/2026-06.sql index 0bbf5d73..c00d4231 100644 --- a/.pql/changelog/ticket_history/2026-06.sql +++ b/.pql/changelog/ticket_history/2026-06.sql @@ -4039,3 +4039,5 @@ INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, chang INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHDAK04ZA0PBT69ZWNBXPSR', 'status', 'in_progress', 'done', NULL, '2026-06-11 23:28:30', '2026-06-11 23:28:30', '2026-06-11 23:28:30', NULL, 'bd49ff4ff2fa7892ab5b667e46236300', 2) ON CONFLICT(hash) DO NOTHING; INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHC7KDFW07S8WTCC3MD71J0', 'status', 'backlog', 'in_progress', NULL, '2026-06-11 23:28:58', '2026-06-11 23:28:58', '2026-06-11 23:28:58', NULL, 'baf6395b93ca8eea9fd9044debffd54d', 2) ON CONFLICT(hash) DO NOTHING; INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHC7KDFW07S8WTCC3MD71J0', 'status', 'in_progress', 'done', NULL, '2026-06-11 23:31:55', '2026-06-11 23:31:55', '2026-06-11 23:31:55', NULL, '06da283bf11516ecae7fd4bd326e470a', 2) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHCEC25337J2AXXQNST56Y4', 'status', 'backlog', 'in_progress', NULL, '2026-06-11 23:32:21', '2026-06-11 23:32:21', '2026-06-11 23:32:21', NULL, '4da867943aa4bb0dea8bdc2699b64423', 2) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHCEC25337J2AXXQNST56Y4', 'status', 'in_progress', 'done', NULL, '2026-06-11 23:35:05', '2026-06-11 23:35:05', '2026-06-11 23:35:05', NULL, 'f1f9f574e58d9142946e8f3267a63e55', 2) ON CONFLICT(hash) DO NOTHING; diff --git a/.pql/changelog/tickets/2026-06.sql b/.pql/changelog/tickets/2026-06.sql index d5f305fc..d645df80 100644 --- a/.pql/changelog/tickets/2026-06.sql +++ b/.pql/changelog/tickets/2026-06.sql @@ -4203,3 +4203,13 @@ INSERT INTO tickets (record_id, type, parent_record_id, title, description, stat Fix: add Terminal.writeBytes(List) backed by a persistent chunked Utf8Decoder (allowMalformed) per terminal instance; migrate the PTY consumer and FileTailFollower to it. Keep write(String) for tests/programmatic use. Acceptance: test feeding a multi-byte rune split across two writeBytes calls renders one glyph; FileTailFollower starting mid-rune resyncs without emitting replacement chars mid-stream.', 'done', 'medium', NULL, NULL, NULL, '2026-06-11 21:57:44', '2026-06-11 23:31:55', NULL, 'fdf8ae8544253d4ae0b79ab17e0d9003', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash); +INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHCEC25337J2AXXQNST56Y4', 'bug', '06FBHBGHNEQTAEPGNJKN42C1E8', 'Extension activation is not transactional; disable ignores dependents; registries clobber on id collision', 'lib/kernel/src/extensions_manager.dart:133-192 — a throw mid-contribution leaves earlier contributions mounted while the extension records as failed; a retry then double-applies them. Also: disabling an extension ignores extensions that depend on it, and contribution registries silently clobber on id collision. Benign among curated builtins; hazardous the day Tier-6 Lua extensions (T-8) land. + +Fix: make activation transactional — collect contributions, mount only after the extension activates cleanly, and unwind mounted ones on failure; disable refuses (or cascades, pick one and record it) when dependents are active; registries reject or namespace duplicate ids with a logged diagnostic. + +Acceptance: test that an extension throwing mid-activation leaves zero contributions mounted and can retry cleanly; disable-with-dependents behaves per the chosen rule; duplicate contribution id surfaces an error instead of clobbering.', 'in_progress', 'medium', NULL, NULL, NULL, '2026-06-11 21:58:40', '2026-06-11 23:32:21', NULL, '5f245fb27177b40eb5b45c90ff7744c3', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash); +INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHCEC25337J2AXXQNST56Y4', 'bug', '06FBHBGHNEQTAEPGNJKN42C1E8', 'Extension activation is not transactional; disable ignores dependents; registries clobber on id collision', 'lib/kernel/src/extensions_manager.dart:133-192 — a throw mid-contribution leaves earlier contributions mounted while the extension records as failed; a retry then double-applies them. Also: disabling an extension ignores extensions that depend on it, and contribution registries silently clobber on id collision. Benign among curated builtins; hazardous the day Tier-6 Lua extensions (T-8) land. + +Fix: make activation transactional — collect contributions, mount only after the extension activates cleanly, and unwind mounted ones on failure; disable refuses (or cascades, pick one and record it) when dependents are active; registries reject or namespace duplicate ids with a logged diagnostic. + +Acceptance: test that an extension throwing mid-activation leaves zero contributions mounted and can retry cleanly; disable-with-dependents behaves per the chosen rule; duplicate contribution id surfaces an error instead of clobbering.', 'done', 'medium', NULL, NULL, NULL, '2026-06-11 21:58:40', '2026-06-11 23:35:05', NULL, 'abc89daa9f0b06a8c29747fc30333b65', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash); diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a50452d..9e11ff97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -142,6 +142,12 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit. rune split across reads into replacement-character noise; the terminal now ingests bytes through a persistent decoder. (T-373) +- **Extension lifecycle is transactional.** A throw mid-activation now + unwinds every contribution it had mounted (a retry no longer + double-applies), deactivating an extension is refused while active + extensions depend on it, and duplicate contribution/command ids are + rejected instead of silently clobbering. (T-377) + - **Accepting ExitPlanMode now leaves plan mode in the conversation panel.** Approving Claude's plan (the ExitPlanMode tool) transitioned the underlying session out of plan mode, but clide's tracked permission mode didn't follow, diff --git a/lib/kernel/src/extensions_manager.dart b/lib/kernel/src/extensions_manager.dart index 339e8a7e..e8dc77e9 100644 --- a/lib/kernel/src/extensions_manager.dart +++ b/lib/kernel/src/extensions_manager.dart @@ -144,10 +144,17 @@ class ExtensionManager extends ChangeNotifier { } } final ctx = _ExtensionContext(manager: this, id: ext.id); + // Transactional: a throw mid-activation must leave NOTHING mounted — + // the old path left earlier contributions live while the extension + // recorded as failed, and a retry double-applied them (T-377). + final applied = []; + var extActivated = false; try { await ext.activate(ctx); + extActivated = true; for (final c in ext.contributions) { _applyContribution(c); + applied.add(c); } // Eagerly load the i18n catalog for any localized tab this extension // contributes, so its title resolves without a "namespace not @@ -165,6 +172,22 @@ class ExtensionManager extends ChangeNotifier { notifyListeners(); log.info('extensions', 'activated $id'); } catch (e, st) { + for (final c in applied.reversed) { + try { + _removeContribution(c); + } catch (e2) { + log.warn('extensions', 'unwind of ${c.id} failed during $id rollback: $e2'); + } + } + if (extActivated) { + // The extension's own activate() succeeded — give it the matching + // teardown so it doesn't hold resources for a failed activation. + try { + await ext.deactivate(); + } catch (e2) { + log.warn('extensions', 'deactivate during $id rollback failed: $e2'); + } + } _failed[id] = e; log.error('extensions', 'activate failed for $id', error: e, stackTrace: st); notifyListeners(); @@ -175,6 +198,17 @@ class ExtensionManager extends ChangeNotifier { if (!_activated.contains(id)) return; final ext = _known[id]; if (ext == null) return; + // Refuse while active extensions depend on this one — deactivating + // underneath them leaves them running against missing services (T-377). + // Disable the dependents first. + final dependents = [ + for (final e in _known.values) + if (_activated.contains(e.id) && e.dependsOn.contains(id)) e.id, + ]; + if (dependents.isNotEmpty) { + log.warn('extensions', 'refusing to deactivate $id: active dependents: ${dependents.join(', ')}'); + return; + } try { await ext.deactivate(); for (final c in ext.contributions) { @@ -196,8 +230,17 @@ class ExtensionManager extends ChangeNotifier { case TabContribution _: case StatusItemContribution _: case ToolbarButtonContribution _: + // Reject duplicates instead of silently mounting a second copy — + // benign among curated builtins, hazardous once third-party + // extensions land (T-377). The throw rolls the activation back. + if (panels.hasContribution(c.id)) { + throw StateError('duplicate contribution id: ${c.id}'); + } panels.contribute(c); case CommandContribution cmd: + if (commands.get(cmd.command) != null) { + throw StateError('duplicate command id: ${cmd.command}'); + } commands.register(cmd); final binding = cmd.defaultBinding; if (binding != null) { diff --git a/lib/kernel/src/panels/registry.dart b/lib/kernel/src/panels/registry.dart index 5ad2be53..4616c9cc 100644 --- a/lib/kernel/src/panels/registry.dart +++ b/lib/kernel/src/panels/registry.dart @@ -30,6 +30,11 @@ class PanelRegistry extends ChangeNotifier { notifyListeners(); } + /// Whether any slot already mounts a contribution with [id]. Used by the + /// extension manager to reject duplicate ids instead of silently mounting + /// a second copy (T-377). + bool hasContribution(String id) => _mounts.values.any((list) => list.any((c) => c.id == id)); + void contribute(ContributionPoint point) { final slot = point.slot; if (slot == null) return; diff --git a/test/kernel/src/extensions_manager_test.dart b/test/kernel/src/extensions_manager_test.dart index fac47d84..da248e83 100644 --- a/test/kernel/src/extensions_manager_test.dart +++ b/test/kernel/src/extensions_manager_test.dart @@ -315,6 +315,86 @@ void main() { expect(ctx.project, same(f.services.project)); expect(ctx.ipc, same(f.services.ipc)); }); + + // T-377: activation is transactional, deactivation respects dependents, + // and duplicate contribution ids are rejected, not silently clobbered. + group('lifecycle hardening (T-377)', () { + test('a throw mid-contribution unwinds everything already mounted', () async { + // The tab mounts first, then the duplicate command id throws. + f.services.commands.register(CommandContribution(id: 'taken', command: 'taken.cmd', run: (_) async => IpcResponse.ok(id: ''))); + f.services.extensions.register( + _Ext( + id: 'half-mounts', + contributions: [ + TabContribution(id: 'half.view', slot: Slots.workspace, title: 'T', build: (_) => const SizedBox.shrink()), + CommandContribution(id: 'half.cmd', command: 'taken.cmd', run: (_) async => IpcResponse.ok(id: '')), + ], + ), + ); + await f.services.extensions.activateAll(); + + expect(f.services.extensions.isActivated('half-mounts'), isFalse); + expect(f.services.extensions.didFail('half-mounts'), isTrue); + expect(f.services.panels.hasContribution('half.view'), isFalse, reason: 'the mounted tab must be unwound'); + }); + + test('a failed activation can retry cleanly without double-applying', () async { + var attempts = 0; + f.services.extensions.register( + _Ext( + id: 'flaky', + contributions: [TabContribution(id: 'flaky.view', slot: Slots.workspace, title: 'T', build: (_) => const SizedBox.shrink())], + onActivate: (_) async { + attempts++; + if (attempts == 1) throw StateError('first attempt fails'); + }, + ), + ); + await f.services.extensions.activateAll(); + expect(f.services.extensions.didFail('flaky'), isTrue); + + await f.services.extensions.activate('flaky'); + expect(f.services.extensions.isActivated('flaky'), isTrue); + expect(f.services.extensions.didFail('flaky'), isFalse); + expect(f.services.panels.tabsFor(Slots.workspace).where((t) => t.id == 'flaky.view'), hasLength(1), reason: 'exactly one mount after the retry'); + }); + + test('deactivate refuses while an active extension depends on it', () async { + f.services.extensions + ..register(_Ext(id: 'base')) + ..register(_Ext(id: 'leaf', dependsOn: const ['base'])); + await f.services.extensions.activateAll(); + + await f.services.extensions.deactivate('base'); + expect(f.services.extensions.isActivated('base'), isTrue, reason: 'refused: leaf still depends on base'); + + await f.services.extensions.deactivate('leaf'); + await f.services.extensions.deactivate('base'); + expect(f.services.extensions.isActivated('base'), isFalse, reason: 'allowed once the dependent is gone'); + }); + + test('a duplicate contribution id fails the second activation', () async { + f.services.extensions + ..register( + _Ext( + id: 'first', + contributions: [TabContribution(id: 'shared.view', slot: Slots.workspace, title: 'A', build: (_) => const SizedBox.shrink())], + ), + ) + ..register( + _Ext( + id: 'second', + contributions: [TabContribution(id: 'shared.view', slot: Slots.workspace, title: 'B', build: (_) => const SizedBox.shrink())], + ), + ); + await f.services.extensions.activateAll(); + + expect(f.services.extensions.isActivated('first'), isTrue); + expect(f.services.extensions.isActivated('second'), isFalse); + expect(f.services.extensions.didFail('second'), isTrue); + expect(f.services.panels.tabsFor(Slots.workspace).where((t) => t.id == 'shared.view'), hasLength(1), reason: 'first-wins, no clobber'); + }); + }); }); }