From 29468602b44181d51c32b867647591d9fe731892 Mon Sep 17 00:00:00 2001 From: Hare Date: Thu, 4 Jun 2026 02:05:47 +0900 Subject: [PATCH] review: approve hook api hardening --- .../artifacts/rereview.md | 52 ++++++++++++++++ .../item.md | 2 +- .../thread.md | 60 +++++++++++++++++++ 3 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 work-items/open/20260603-122317-hook-public-surface-hardening/artifacts/rereview.md diff --git a/work-items/open/20260603-122317-hook-public-surface-hardening/artifacts/rereview.md b/work-items/open/20260603-122317-hook-public-surface-hardening/artifacts/rereview.md new file mode 100644 index 00000000..a43efd50 --- /dev/null +++ b/work-items/open/20260603-122317-hook-public-surface-hardening/artifacts/rereview.md @@ -0,0 +1,52 @@ +# External rereview: hook public surface hardening + +## 1. Result: approve + +Approve. The previous blocker is fixed: the public pre-tool hook action surface no longer exposes a no-result skip action, and public deny still maps to a synthetic error result. + +## 2. Summary of rereviewed changes + +The follow-up commit `a4e30e2 fix: remove public hook skip action` narrowed `HookPreToolAction` by removing the `Skip` variant and its conversion to `llm_worker::interceptor::PreToolAction::Skip`. The public pre-tool actions are now `Continue`, `Deny(String)`, `Abort(String)`, and `Pause`. + +`HookPreToolAction::Deny` still converts internally to `PreToolAction::SyntheticResult(ToolResult::error(call_id, reason))`, preserving the fail-closed/synthetic-result path needed by manifest permissions and future public hooks. + +## 3. Requirement-by-requirement assessment + +- Previous blocker fixed: satisfied. `HookPreToolAction::Skip` is gone, and grep found no `HookPreToolAction::Skip`, `PreToolAction::Skip`, or standalone `Skip` usage under `crates/pod/src`. +- Public pre-tool deny maps to synthetic error result: satisfied. `HookPreToolAction::Deny` converts to `PreToolAction::SyntheticResult(ToolResult::error(...))`; existing interceptor tests also assert the synthetic result has the expected call id, summary, no content, and `is_error = true`. +- Public Hook API exposes no no-result skip: satisfied. The only no-result skip capability remains in the lower-level `llm_worker` interceptor model; it is no longer reachable through the public `pod::hook::HookPreToolAction` surface. +- Public Hook API exposes no raw `Item` injection: satisfied. `PreLlmRequest` and `OnTurnEnd` use safe public action types rather than raw `PreRequestAction::ContinueWith(Vec)` or `TurnEndAction::ContinueWithMessages(Vec)` as hook outputs. +- Public Hook API exposes no arbitrary `ToolResult` construction: satisfied for action outputs. Public pre-tool hooks provide only a denial message; Pod constructs the synthetic error result internally. +- Internal capabilities remain internal: satisfied. Internal Pod/Worker code still uses richer `llm_worker::Interceptor` actions where needed, including durable host-owned prompt append paths and compact/internal mechanisms. +- Tests cover the fixed path sufficiently: satisfied. The added `public_pre_tool_hook_actions_cannot_emit_internal_no_result_skip` unit test verifies the available public pre-tool conversions and asserts public deny produces a synthetic result. The existing `public_pre_tool_hook_deny_becomes_synthetic_error_and_short_circuits` integration-style interceptor test covers bridge behavior and ordering. + +## 4. Blockers + +None. + +## 5. Non-blockers / follow-ups + +- As noted in the original review, `ToolResultSummary` still exposes `llm_worker::tool::ToolOutput` as public hook input. This is not a blocker because it does not allow output rewriting or arbitrary `ToolResult` construction, but the future plugin/feature registry may still want a Pod-owned summary type before freezing a public API boundary. + +## 6. Validation assessed or rerun + +Rerun/read-only checks from `/home/hare/Projects/yoi/.worktree/hook-public-surface-hardening`: + +- `git rev-parse HEAD` reported `a4e30e292abf5c640b923e3307a75eded366351a`. +- `git status --short` was clean. +- `git diff --check develop...HEAD` passed. +- `cargo fmt --check` passed. +- `./tickets.sh doctor` passed. +- `git grep -n -E 'HookPreToolAction::Skip|PreToolAction::Skip|pub (use|type).*\b(PreRequestAction|TurnEndAction|PreToolAction|PostToolAction|ToolResult|Item)\b' crates/pod/src` produced no matches. + +Assessed by source review: + +- Follow-up commit `a4e30e2` diff. +- Full diff `develop...HEAD` for `crates/pod/src/hook.rs`, `crates/pod/src/ipc/interceptor.rs`, `crates/pod/src/permission.rs`, and `crates/pod/src/pod.rs`. +- Relevant lower-level `llm-worker` pre-tool handling to confirm internal skip remains lower-level only. + +I did not run `cargo test`, `cargo check`, or `nix build`; those commands would write build artifacts outside this review artifact scope. The fixed-path tests were assessed from source. + +## 7. Residual risk + +No merge-blocking residual risk found. The remaining risk is API polish for the future feature/plugin registry, especially whether all hook input summary types should become Pod-owned wrapper types before being treated as a stable plugin boundary. diff --git a/work-items/open/20260603-122317-hook-public-surface-hardening/item.md b/work-items/open/20260603-122317-hook-public-surface-hardening/item.md index 09ae1ef0..2da66029 100644 --- a/work-items/open/20260603-122317-hook-public-surface-hardening/item.md +++ b/work-items/open/20260603-122317-hook-public-surface-hardening/item.md @@ -7,7 +7,7 @@ kind: task priority: P1 labels: [hooks, plugin, safety, tests] created_at: 2026-06-03T12:23:17Z -updated_at: 2026-06-03T16:58:49Z +updated_at: 2026-06-03T17:05:43Z assignee: null legacy_ticket: null --- diff --git a/work-items/open/20260603-122317-hook-public-surface-hardening/thread.md b/work-items/open/20260603-122317-hook-public-surface-hardening/thread.md index 983f8abf..5fdea1a3 100644 --- a/work-items/open/20260603-122317-hook-public-surface-hardening/thread.md +++ b/work-items/open/20260603-122317-hook-public-surface-hardening/thread.md @@ -169,4 +169,64 @@ I did not run `cargo test`, `cargo check`, or `nix build`; those commands would After fixing the public skip path, the main remaining risk is API shape stabilization for the future plugin/feature registry: public hook inputs still share some worker/tool data types, and those should be deliberately frozen or wrapped before becoming a long-term plugin ABI. The raw `Item` injection risk for pre-request and turn-end hooks appears addressed by this implementation. +--- + + + +## Review: approve + +# External rereview: hook public surface hardening + +## 1. Result: approve + +Approve. The previous blocker is fixed: the public pre-tool hook action surface no longer exposes a no-result skip action, and public deny still maps to a synthetic error result. + +## 2. Summary of rereviewed changes + +The follow-up commit `a4e30e2 fix: remove public hook skip action` narrowed `HookPreToolAction` by removing the `Skip` variant and its conversion to `llm_worker::interceptor::PreToolAction::Skip`. The public pre-tool actions are now `Continue`, `Deny(String)`, `Abort(String)`, and `Pause`. + +`HookPreToolAction::Deny` still converts internally to `PreToolAction::SyntheticResult(ToolResult::error(call_id, reason))`, preserving the fail-closed/synthetic-result path needed by manifest permissions and future public hooks. + +## 3. Requirement-by-requirement assessment + +- Previous blocker fixed: satisfied. `HookPreToolAction::Skip` is gone, and grep found no `HookPreToolAction::Skip`, `PreToolAction::Skip`, or standalone `Skip` usage under `crates/pod/src`. +- Public pre-tool deny maps to synthetic error result: satisfied. `HookPreToolAction::Deny` converts to `PreToolAction::SyntheticResult(ToolResult::error(...))`; existing interceptor tests also assert the synthetic result has the expected call id, summary, no content, and `is_error = true`. +- Public Hook API exposes no no-result skip: satisfied. The only no-result skip capability remains in the lower-level `llm_worker` interceptor model; it is no longer reachable through the public `pod::hook::HookPreToolAction` surface. +- Public Hook API exposes no raw `Item` injection: satisfied. `PreLlmRequest` and `OnTurnEnd` use safe public action types rather than raw `PreRequestAction::ContinueWith(Vec)` or `TurnEndAction::ContinueWithMessages(Vec)` as hook outputs. +- Public Hook API exposes no arbitrary `ToolResult` construction: satisfied for action outputs. Public pre-tool hooks provide only a denial message; Pod constructs the synthetic error result internally. +- Internal capabilities remain internal: satisfied. Internal Pod/Worker code still uses richer `llm_worker::Interceptor` actions where needed, including durable host-owned prompt append paths and compact/internal mechanisms. +- Tests cover the fixed path sufficiently: satisfied. The added `public_pre_tool_hook_actions_cannot_emit_internal_no_result_skip` unit test verifies the available public pre-tool conversions and asserts public deny produces a synthetic result. The existing `public_pre_tool_hook_deny_becomes_synthetic_error_and_short_circuits` integration-style interceptor test covers bridge behavior and ordering. + +## 4. Blockers + +None. + +## 5. Non-blockers / follow-ups + +- As noted in the original review, `ToolResultSummary` still exposes `llm_worker::tool::ToolOutput` as public hook input. This is not a blocker because it does not allow output rewriting or arbitrary `ToolResult` construction, but the future plugin/feature registry may still want a Pod-owned summary type before freezing a public API boundary. + +## 6. Validation assessed or rerun + +Rerun/read-only checks from `/home/hare/Projects/yoi/.worktree/hook-public-surface-hardening`: + +- `git rev-parse HEAD` reported `a4e30e292abf5c640b923e3307a75eded366351a`. +- `git status --short` was clean. +- `git diff --check develop...HEAD` passed. +- `cargo fmt --check` passed. +- `./tickets.sh doctor` passed. +- `git grep -n -E 'HookPreToolAction::Skip|PreToolAction::Skip|pub (use|type).*\b(PreRequestAction|TurnEndAction|PreToolAction|PostToolAction|ToolResult|Item)\b' crates/pod/src` produced no matches. + +Assessed by source review: + +- Follow-up commit `a4e30e2` diff. +- Full diff `develop...HEAD` for `crates/pod/src/hook.rs`, `crates/pod/src/ipc/interceptor.rs`, `crates/pod/src/permission.rs`, and `crates/pod/src/pod.rs`. +- Relevant lower-level `llm-worker` pre-tool handling to confirm internal skip remains lower-level only. + +I did not run `cargo test`, `cargo check`, or `nix build`; those commands would write build artifacts outside this review artifact scope. The fixed-path tests were assessed from source. + +## 7. Residual risk + +No merge-blocking residual risk found. The remaining risk is API polish for the future feature/plugin registry, especially whether all hook input summary types should become Pod-owned wrapper types before being treated as a stable plugin boundary. + + ---