diff --git a/work-items/open/20260603-122317-plugin-feature-contribution-registry/artifacts/rereview.md b/work-items/open/20260603-122317-plugin-feature-contribution-registry/artifacts/rereview.md new file mode 100644 index 00000000..0d089e07 --- /dev/null +++ b/work-items/open/20260603-122317-plugin-feature-contribution-registry/artifacts/rereview.md @@ -0,0 +1,100 @@ +# External sibling rereview: plugin-feature-contribution-registry + +## 1. Result: request changes + +Request changes. The second blocker from the original review is addressed, but the first blocker is only partially fixed: the registry now compares the wrapper name against one materialization of the `ToolDefinition`, then queues the original factory for a second Worker materialization, so the checked name can still diverge from the actual model-visible registered tool name. + +## 2. Summary of rereview + +Reviewed the existing worktree/branch: + +- Worktree: `/home/hare/Projects/yoi/.worktree/plugin-feature-contribution-registry` +- Branch: `work/plugin-feature-contribution-registry` +- Commits reviewed: + - `a8ae6ca feat: add pod feature registry slice` + - `4070176 fix: harden feature contribution gates` + +The fix commit changes only `crates/pod/src/feature.rs`. It adds capability checks for background task, service, notification, alert, and diagnostic surfaces; adds service requirement capability checks; adds tool-name mismatch diagnostics; and expands focused tests. + +## 3. Prior blocker assessment + +### Prior blocker 1: Tool wrapper name vs actual model-visible tool name + +Status: **not fully resolved**. + +The new code in `ToolContributionRegistrar::register` does verify the wrapper name against a `ToolDefinition` materialization: + +```rust +let model_visible_name = (contribution.definition)().0.name; +if contribution.name != model_visible_name { ... } +``` + +It then performs capability checks, duplicate checks, skipped/report entries, and `installed_tools` against that `model_visible_name`, which fixes the simple stable-factory mismatch case and is covered by the new test `mismatched_tool_contribution_name_is_rejected_before_queueing`. + +However, after that validation it queues the original `ToolDefinition` factory unchanged: + +```rust +self.pending_tools.push(contribution.definition); +``` + +`ToolDefinition` is an arbitrary `Arc (ToolMeta, Arc) + Send + Sync>`, and its own documentation says it is called once during Worker registration. This implementation now calls it once for registry validation and then lets Worker call it again during `flush_pending`. A non-idempotent or stateful factory can return `ToolMeta.name = "Checked"` during registry validation and `ToolMeta.name = "ActuallyRegistered"` during Worker registration. In that case capability checks, duplicate checks, skipped/report entries, and installed tool reports are keyed to the first name, while the model-visible Worker tool is registered under the second name. + +This keeps the original boundary hole open for the actual Worker queueing path. It is also a behavior risk for factories whose construction has side effects, since the first materialized tool instance is discarded. + +Before merge, the registry should make the validated tool identity the same materialization that Worker registers. Acceptable shapes include: + +- materialize `(ToolMeta, Arc)` once in the feature registry, validate `ToolMeta.name`, then queue a stable factory that returns the already validated metadata/tool instance; or +- change the lower Worker/tool registration path to accept a materialized tool registration type; or +- otherwise enforce a type-level/single-materialization invariant so the registry-checked name cannot differ from the Worker-registered name. + +Add a regression test with a `ToolDefinition` that returns different names across calls; it should not be possible for the report to say one name while the Worker registers another. + +### Prior blocker 2: Capability grants for non-tool/hook surfaces + +Status: **resolved for this Phase 1/2 slice**. + +The fix adds capability enforcement or explicit denial/skipping for the previously uncovered surfaces: + +- `BackgroundTaskRegistrar::declare` now checks `DeclareBackgroundTask` through `require_background_task_capability`. +- Descriptor-declared background tasks are also checked before being recorded in `declared_background_tasks`. +- `FeatureServiceRegistrar::provide` now checks `ProvideService` through `require_service_provider_capability`. +- Descriptor-declared service providers are also checked before registration in `FeatureServiceRegistry`. +- Service requirements are checked against `RequireService`; required missing/denied requirements block installation, while optional missing/denied requirements are skipped/degraded. +- `FeatureNotificationSink`, `FeatureAlertSink`, and `FeatureDiagnosticSink` now require `EmitNotification`, `EmitAlert`, and `EmitDiagnostic` respectively before recording their skeleton/report-only output. + +The install path still uses `CapabilityGrantSet::grant_all(&descriptor.requested_capabilities)`, so this slice is not a real host policy resolver yet. That is acceptable for the focused registry skeleton as long as absent/unrequested capabilities are denied/skipped, which the updated code now does. + +## 4. Blockers + +### Blocker — ToolDefinition is validated once but registered from a second factory call + +The registry must not validate/report one tool name while Worker can register another. The current fix validates one call to the `ToolDefinition` but queues the same factory for a later Worker call, so `ToolMeta.name` can still diverge between registry checks and actual model-visible registration. + +This must be fixed before merge because it directly affects the requested invariant: capability checks, duplicate checks, skipped/reports, and Worker queueing must share the same model-visible tool identity. + +## 5. Non-blockers / follow-ups + +- The new tests cover the stable-factory mismatch case, background-task capability denial, service-provider capability denial, service requirement basics, duplicate tool names, and Task tool registration. They do not yet cover notification/alert/diagnostic sink capability denial. The code path is straightforward, so I do not classify this as a blocker, but adding a small sink-denial test would make the capability-surface coverage more complete. +- The service resolution remains installation-order based. This was already noted in the original review and remains acceptable as a follow-up for this descriptor/skeleton slice. +- The install report is still not surfaced outside local `_feature_install_report` plumbing. This remains acceptable for the small built-in proof but should be addressed when discovery/enablement diagnostics become user-visible. + +## 6. Validation assessed or rerun + +Reran: + +- `git diff --check develop...HEAD` — passed. +- `cargo test -p pod feature::tests --lib` — passed: 7 tests, 0 failures. + +Assessed by inspection: + +- Original review artifact. +- Ticket item and delegation intent. +- `git diff a8ae6ca..HEAD -- crates/pod/src/feature.rs`. +- Current `crates/pod/src/feature.rs` around tool registration, capability gates, service/background handling, notification/alert/diagnostic sinks, and tests. +- Search for generic event/UI/context/history surfaces in `crates/pod/src/feature.rs`. + +I did not rerun full `cargo test -p pod --lib`, workspace check, or `nix build .#yoi` in this rereview because the remaining issue is visible by focused inspection and the requested scope was blocker re-review. + +## 7. Residual risk + +No new generic event channel, custom UI/dialog path, hidden context/history injection, raw `Pod`, raw `ToolServerHandle`, raw `Interceptor`, raw history writer, raw event sender, or raw `NotifyBuffer` exposure was found in the feature API changes. Once the tool materialization/registration identity issue is fixed, the remaining risk looks appropriate for the intended descriptor-first Phase 1/2 slice. \ No newline at end of file diff --git a/work-items/open/20260603-122317-plugin-feature-contribution-registry/item.md b/work-items/open/20260603-122317-plugin-feature-contribution-registry/item.md index e2697e79..0a5a278b 100644 --- a/work-items/open/20260603-122317-plugin-feature-contribution-registry/item.md +++ b/work-items/open/20260603-122317-plugin-feature-contribution-registry/item.md @@ -7,7 +7,7 @@ kind: feature priority: P1 labels: [plugin, registry, tools, hooks, orchestration] created_at: 2026-06-03T12:23:17Z -updated_at: 2026-06-04T21:32:42Z +updated_at: 2026-06-04T21:46:04Z assignee: null legacy_ticket: null --- diff --git a/work-items/open/20260603-122317-plugin-feature-contribution-registry/thread.md b/work-items/open/20260603-122317-plugin-feature-contribution-registry/thread.md index 926ba17d..c4b78ca3 100644 --- a/work-items/open/20260603-122317-plugin-feature-contribution-registry/thread.md +++ b/work-items/open/20260603-122317-plugin-feature-contribution-registry/thread.md @@ -867,3 +867,110 @@ I did not rerun `cargo test`, `cargo check`, or `nix build` in this external rev After the blockers are fixed, the main residual risk is that this first registry slice remains descriptor-heavy while actual host policy resolution is still minimal. That is acceptable for Phase 1/2 if the public API cannot misrepresent model-visible tools and if every exposed contribution/sink consistently goes through capability checks or is explicitly deferred. --- + + + +## Review: request changes + +# External sibling rereview: plugin-feature-contribution-registry + +## 1. Result: request changes + +Request changes. The second blocker from the original review is addressed, but the first blocker is only partially fixed: the registry now compares the wrapper name against one materialization of the `ToolDefinition`, then queues the original factory for a second Worker materialization, so the checked name can still diverge from the actual model-visible registered tool name. + +## 2. Summary of rereview + +Reviewed the existing worktree/branch: + +- Worktree: `/home/hare/Projects/yoi/.worktree/plugin-feature-contribution-registry` +- Branch: `work/plugin-feature-contribution-registry` +- Commits reviewed: + - `a8ae6ca feat: add pod feature registry slice` + - `4070176 fix: harden feature contribution gates` + +The fix commit changes only `crates/pod/src/feature.rs`. It adds capability checks for background task, service, notification, alert, and diagnostic surfaces; adds service requirement capability checks; adds tool-name mismatch diagnostics; and expands focused tests. + +## 3. Prior blocker assessment + +### Prior blocker 1: Tool wrapper name vs actual model-visible tool name + +Status: **not fully resolved**. + +The new code in `ToolContributionRegistrar::register` does verify the wrapper name against a `ToolDefinition` materialization: + +```rust +let model_visible_name = (contribution.definition)().0.name; +if contribution.name != model_visible_name { ... } +``` + +It then performs capability checks, duplicate checks, skipped/report entries, and `installed_tools` against that `model_visible_name`, which fixes the simple stable-factory mismatch case and is covered by the new test `mismatched_tool_contribution_name_is_rejected_before_queueing`. + +However, after that validation it queues the original `ToolDefinition` factory unchanged: + +```rust +self.pending_tools.push(contribution.definition); +``` + +`ToolDefinition` is an arbitrary `Arc (ToolMeta, Arc) + Send + Sync>`, and its own documentation says it is called once during Worker registration. This implementation now calls it once for registry validation and then lets Worker call it again during `flush_pending`. A non-idempotent or stateful factory can return `ToolMeta.name = "Checked"` during registry validation and `ToolMeta.name = "ActuallyRegistered"` during Worker registration. In that case capability checks, duplicate checks, skipped/report entries, and installed tool reports are keyed to the first name, while the model-visible Worker tool is registered under the second name. + +This keeps the original boundary hole open for the actual Worker queueing path. It is also a behavior risk for factories whose construction has side effects, since the first materialized tool instance is discarded. + +Before merge, the registry should make the validated tool identity the same materialization that Worker registers. Acceptable shapes include: + +- materialize `(ToolMeta, Arc)` once in the feature registry, validate `ToolMeta.name`, then queue a stable factory that returns the already validated metadata/tool instance; or +- change the lower Worker/tool registration path to accept a materialized tool registration type; or +- otherwise enforce a type-level/single-materialization invariant so the registry-checked name cannot differ from the Worker-registered name. + +Add a regression test with a `ToolDefinition` that returns different names across calls; it should not be possible for the report to say one name while the Worker registers another. + +### Prior blocker 2: Capability grants for non-tool/hook surfaces + +Status: **resolved for this Phase 1/2 slice**. + +The fix adds capability enforcement or explicit denial/skipping for the previously uncovered surfaces: + +- `BackgroundTaskRegistrar::declare` now checks `DeclareBackgroundTask` through `require_background_task_capability`. +- Descriptor-declared background tasks are also checked before being recorded in `declared_background_tasks`. +- `FeatureServiceRegistrar::provide` now checks `ProvideService` through `require_service_provider_capability`. +- Descriptor-declared service providers are also checked before registration in `FeatureServiceRegistry`. +- Service requirements are checked against `RequireService`; required missing/denied requirements block installation, while optional missing/denied requirements are skipped/degraded. +- `FeatureNotificationSink`, `FeatureAlertSink`, and `FeatureDiagnosticSink` now require `EmitNotification`, `EmitAlert`, and `EmitDiagnostic` respectively before recording their skeleton/report-only output. + +The install path still uses `CapabilityGrantSet::grant_all(&descriptor.requested_capabilities)`, so this slice is not a real host policy resolver yet. That is acceptable for the focused registry skeleton as long as absent/unrequested capabilities are denied/skipped, which the updated code now does. + +## 4. Blockers + +### Blocker — ToolDefinition is validated once but registered from a second factory call + +The registry must not validate/report one tool name while Worker can register another. The current fix validates one call to the `ToolDefinition` but queues the same factory for a later Worker call, so `ToolMeta.name` can still diverge between registry checks and actual model-visible registration. + +This must be fixed before merge because it directly affects the requested invariant: capability checks, duplicate checks, skipped/reports, and Worker queueing must share the same model-visible tool identity. + +## 5. Non-blockers / follow-ups + +- The new tests cover the stable-factory mismatch case, background-task capability denial, service-provider capability denial, service requirement basics, duplicate tool names, and Task tool registration. They do not yet cover notification/alert/diagnostic sink capability denial. The code path is straightforward, so I do not classify this as a blocker, but adding a small sink-denial test would make the capability-surface coverage more complete. +- The service resolution remains installation-order based. This was already noted in the original review and remains acceptable as a follow-up for this descriptor/skeleton slice. +- The install report is still not surfaced outside local `_feature_install_report` plumbing. This remains acceptable for the small built-in proof but should be addressed when discovery/enablement diagnostics become user-visible. + +## 6. Validation assessed or rerun + +Reran: + +- `git diff --check develop...HEAD` — passed. +- `cargo test -p pod feature::tests --lib` — passed: 7 tests, 0 failures. + +Assessed by inspection: + +- Original review artifact. +- Ticket item and delegation intent. +- `git diff a8ae6ca..HEAD -- crates/pod/src/feature.rs`. +- Current `crates/pod/src/feature.rs` around tool registration, capability gates, service/background handling, notification/alert/diagnostic sinks, and tests. +- Search for generic event/UI/context/history surfaces in `crates/pod/src/feature.rs`. + +I did not rerun full `cargo test -p pod --lib`, workspace check, or `nix build .#yoi` in this rereview because the remaining issue is visible by focused inspection and the requested scope was blocker re-review. + +## 7. Residual risk + +No new generic event channel, custom UI/dialog path, hidden context/history injection, raw `Pod`, raw `ToolServerHandle`, raw `Interceptor`, raw history writer, raw event sender, or raw `NotifyBuffer` exposure was found in the feature API changes. Once the tool materialization/registration identity issue is fixed, the remaining risk looks appropriate for the intended descriptor-first Phase 1/2 slice. + +---