feat(reborn): add ironclaw_hooks framework foundation (#3524) - #3573
Merged
Merged
Conversation
Foundation slice of the Reborn loop hooks framework per #3524. Lands the trust primitives, sealed decision types, dispatcher contract, and extension manifest schema; no Reborn middleware composition yet (next slice wires HookDispatcher into LoopCapabilityPort / LoopPromptPort). Design comment on #3524: #3524 (comment) What this PR ships ================== * `crates/ironclaw_hooks/` — new crate * `identity` — content-addressed `HookId` (blake3 of length-prefixed extension + local + version fields). Same versioning primitive the rest of Reborn should converge on for replay safety. * `trust` — `HookTrustClass` enum (Builtin / Trusted / Installed) with per-kind default attenuation. Trust class is fixed by source, never declarable. * `kinds/` — sealed decision DTOs. `BeforeCapabilityHookDecision`, `HookPatch`, `ObserverFact` all have `pub` outer struct + `pub(crate)` inner enum + `pub(crate)` constructors. Same #3460 witness pattern. * `points/` — typed read-only contexts for each hook point. * `sink` — split sink traits per trust tier. `PrivilegedGateSink` exposes `allow()`; `RestrictedGateSink` does not. An Installed-tier hook literally cannot mint Allow at the type level. * `ordering` — phase → priority → hook id, stable. Phases gated by trust (Validation/Authorization Builtin-only). * `failure_policy` — Timeout/Panic/Malformed/AttenuationViolation categories. Gate/Mutator fail closed, Observer/Effect fail isolated. Slot poisoning persisted for the rest of the run on any category. * `registry` — run-profile-sourced bindings; phase-vs-trust gate enforced at insert; poisoning surface for the dispatcher. * `dispatch` — HookDispatcher with deterministic ordering, panic catch-unwind via futures::FutureExt, per-hook tokio::time::timeout, short-circuit gate composition (Deny > PauseAuth > PauseApproval > Allow), Telemetry-phase observers always run. * `manifest` — serde types for the `[[hooks]]` section of extension manifests. Predicate vs WASM body; same_tenant scope requires explicit grant; Validation/Authorization phases rejected at parse time because manifest hooks are always Installed. * `predicate` — typed predicate language for declarative Installed hooks (DenyCapability, PauseApproval, RateOrValueCap). Evaluator lives in the dispatcher follow-up, not here. * `crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs` * Added `ironclaw_turns` -> `ironclaw_hooks` to the forbidden list. * New BoundaryRule for `ironclaw_hooks` itself (cannot pull host_runtime, dispatcher, secrets, network, wasm, etc.). * `Cargo.toml` workspace member registration. What this PR deliberately does NOT ship ======================================== * Reborn middleware composition wrapping LoopCapabilityPort / LoopPromptPort with HookDispatcher. Next slice; ironclaw_reborn changes only. * WASM hook execution path. Programmatic hooks parse and validate from manifest; the wasmtime integration lands when the WASM dispatcher seam is built. * Predicate evaluation. Predicate types serialize and validate; the evaluator that turns a `RateOrValueCap` spec into a `Deny` decision is in the next slice alongside Reborn wiring. * Event-triggered hooks (Phase 5 of the original roadmap). * Self-authored hooks. Tracked separately at #3567 with monotonic-restriction + unforgeable-channel ratification. Test plan ========= * `cargo test -p ironclaw_hooks` — 47 tests (46 unit + 1 integration smoke for the manifest -> binding -> dispatch pipeline). * `cargo test -p ironclaw_architecture` — 13 tests; new boundary rule passes, existing rules unaffected. * `cargo clippy -p ironclaw_hooks --all-targets -- -D warnings` — clean. * `cargo fmt -p ironclaw_hooks -- --check` — clean. * `cargo check --workspace` — clean, no regressions in other crates. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
scope: docs
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the ironclaw_hooks crate, a framework for managing inline and event-triggered hooks within the Reborn loop. It establishes a trust-tiered model (Builtin, Trusted, Installed) using sealed decision types and sink traits to enforce security invariants at the type level. The implementation includes a hook dispatcher with panic isolation and timeout enforcement, a content-addressed identity system, and a declarative predicate language for extension-based hooks. Review feedback highlights the need to include the priority field in HookBinding to ensure deterministic ordering, corrects the temporal logic of observer dispatches, and suggests performance optimizations for hex string generation.
| pub struct HookBinding { | ||
| pub hook_id: HookId, | ||
| pub hook_version: HookVersion, | ||
| pub trust_class: HookTrustClass, |
Contributor
There was a problem hiding this comment.
The HookBinding struct is missing the priority field. Since the dispatcher's ordering logic relies on HookOrderKey which includes priority, and HookManifestEntry allows authors to specify it, this information must be preserved in the binding to ensure deterministic ordering as designed. Note that you will also need to update the struct initialization in tests (e.g., lines 135-142).
Suggested change
| pub trust_class: HookTrustClass, | |
| pub phase: HookPhase, | |
| pub priority: crate::ordering::HookPriority, |
Collaborator
Author
There was a problem hiding this comment.
Fixed — HookBinding.priority field added in d9b2000 with #[serde(default = "default_priority")] for checkpoint back-compat. Re-applied in e6288a5 after a base re-merge reverted it (Firat caught the same issue independently). The registrar wires the manifest's priority via HookDispatcher::set_binding_priority post-install.
| .cloned() | ||
| .map(|b| { | ||
| let key = | ||
| HookOrderKey::new(b.phase, crate::ordering::HookPriority::DEFAULT, b.hook_id); |
Contributor
There was a problem hiding this comment.
Once priority is added to HookBinding, it should be used here instead of hardcoding HookPriority::DEFAULT. This ensures that the author's chosen priority is respected during dispatch.
Suggested change
| HookOrderKey::new(b.phase, crate::ordering::HookPriority::DEFAULT, b.hook_id); | |
| HookOrderKey::new(b.phase, b.priority, b.hook_id); |
Collaborator
Author
There was a problem hiding this comment.
Fixed in e6288a5 — ordered_bindings now uses b.priority exactly as suggested. Regression test priority_overrides_hook_id_tiebreaker_in_dispatch_order covers the FIRST-vs-LAST case.
Comment on lines
+48
to
+53
| let mut s = String::with_capacity(64); | ||
| for byte in self.0 { | ||
| s.push_str(&format!("{byte:02x}")); | ||
| } | ||
| s | ||
| } |
Contributor
There was a problem hiding this comment.
Using push_str with format! inside a loop causes 32 string allocations per call. For a foundation crate, it is more efficient to use std::fmt::Write to write directly into the pre-allocated buffer to avoid unnecessary heap allocations.
pub fn to_hex(&self) -> String {
use std::fmt::Write;
let mut s = String::with_capacity(64);
for byte in self.0 {
let _ = write!(s, "{byte:02x}");
}
s
}References
- To improve performance, avoid unnecessary heap allocations by using iterators or writing directly to a pre-allocated buffer.
Collaborator
Author
There was a problem hiding this comment.
Fixed in d9b2000 — switched to_hex to std::fmt::Write::write! into the pre-allocated buffer, eliminating the 32 throwaway String allocations per call. Same fix applied to fmt::Debug (now writes directly to the formatter; no intermediate String).
Comment on lines
+202
to
+204
| let telemetry_outcome = self | ||
| .dispatch_observer_at(HookPointSpec::AfterCapability, ctx.tenant_id.clone()) | ||
| .await; |
Contributor
There was a problem hiding this comment.
Dispatching AfterCapability observers inside dispatch_before_capability is temporally incorrect. AfterCapability hooks should be triggered by the loop driver after the capability has actually been invoked (or explicitly denied). BeforeCapability already provides a Telemetry phase for observers that need to run during the pre-invocation phase.
Collaborator
Author
There was a problem hiding this comment.
Comment on lines
+273
to
+278
| _ => { | ||
| // Non-observer point passed in; return empty outcome and | ||
| // record a protocol violation against the dispatcher's own | ||
| // configuration (this is a bug in the caller). | ||
| return ObserverDispatchOutcome { facts, failures }; | ||
| } |
Contributor
There was a problem hiding this comment.
The catch-all arm for non-observer points mentions recording a protocol violation but currently just returns an empty outcome. Since this indicates a bug in the dispatcher's caller, it should log an error to aid debugging. Ensure the log message does not reference internal implementation details like function names.
_ => {
tracing::error!(?point, "invalid observer point encountered during dispatch");
return ObserverDispatchOutcome { facts, failures };
}References
- Avoid coupling log messages to implementation details like configuration interfaces or source code layout.
Collaborator
Author
There was a problem hiding this comment.
Fixed in d9b2000 — catch-all arm now emits tracing::error! with the offending point field, so the dispatcher-wiring bug surfaces in logs without crashing the loop.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdf173d174
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+124
to
+125
| pub fn install_before_capability(&mut self, hook_id: HookId, hook: BeforeCapabilityHookImpl) { | ||
| self.before_capability.insert(hook_id, hook); |
There was a problem hiding this comment.
install_before_capability accepts any BeforeCapabilityHookImpl for a hook_id and never checks that the impl variant matches the binding’s trust_class. If the installer or runtime wiring accidentally registers an Installed hook as Privileged, dispatch will execute it with a privileged sink, allowing it to call methods (like allow) that the trust model says must be unreachable for installed hooks.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
Type-enforced at compile time, not runtime. install_before_capability takes Arc<dyn BeforeCapabilityHookImpl> whose only constructors are the sealed variants Privileged/Restricted/Installed/SelfAuthored, each of which carries its own sink type. An Installed impl literally cannot hold a privileged sink — the sealed-trait/enum pairing makes that unrepresentable.
Evidence:
crates/ironclaw_hooks/src/dispatch.rs:1822—compile_time_seal_testasserts the sealing.crates/ironclaw_hooks/src/dispatch.rs:1894—installed_binding_cannot_be_paired_with_privileged_implproves the cross-binding/impl pairing is rejected.
If you spot a constructor path that lets an Installed-tier value carry a privileged sink, flag the exact call site and we'll add coverage.
Comment on lines
+309
to
+311
| let key = | ||
| HookOrderKey::new(b.phase, crate::ordering::HookPriority::DEFAULT, b.hook_id); | ||
| (key, b) |
There was a problem hiding this comment.
Dispatch ordering currently hardcodes every binding to HookPriority::DEFAULT, so non-default priorities from manifest/installer input are dropped and execution order collapses to (phase, hook_id). This breaks the documented (phase, priority, hook_id) contract and can reorder policy hooks in ways that change effective behavior when multiple hooks rely on explicit priority.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
Fixed in e6288a502. ordered_bindings now uses b.priority instead of hardcoding HookPriority::DEFAULT, restoring the documented (phase, priority, hook_id) contract. Regression test priority_overrides_hook_id_tiebreaker_in_dispatch_order in crates/ironclaw_hooks/src/dispatch.rs pins the behavior.
Follows the foundation slice (see initial commit). Adds the next layer:
1. Capability- and prompt-port middleware (`ironclaw_hooks::middleware`)
* `HookedLoopCapabilityPort` runs `dispatch_before_capability` before
every invocation, translates the composed decision into the existing
`CapabilityOutcome` vocabulary (Deny / PauseApproval / PauseAuth all
map to `Denied` for now; gate-ref plumbing for real pause semantics
lands in the next slice).
* `HookedLoopPromptPort` runs `dispatch_before_prompt` before bundle
construction. Observe-only for snippets in this slice; actual
snippet injection waits for the shared `prompt_envelope::wrap_untrusted`
helper (#3540 / #3471).
2. Declarative predicate evaluator (`ironclaw_hooks::evaluator`)
* `DenyCapability` and `PauseApproval` predicates: stateless, evaluated
directly against `BeforeCapabilityHookContext`.
* `RateOrValueCap` with `InvocationCount` bound: sliding-window counter
keyed by `(hook_id, capability_name)`, in-memory only. Window
parsing supports `s`/`m`/`h`/`d` units; unparseable windows fail
closed.
* `NumericSum` bound: types implemented but evaluation returns Allow
and emits a warn-level audit. Full argument-extraction story is a
follow-up slice once capability arguments become hook-visible.
* `PredicateEvaluator::evaluate_at(...)` test variant accepts an
explicit `Instant` so sliding-window tests don't depend on
real-clock progress.
3. Manifest -> dispatcher glue (`ironclaw_hooks::installed_hook`)
* `PredicateBackedBeforeCapabilityHook` wraps a `HookPredicateSpec`
plus an `Arc<PredicateEvaluator>` and implements
`RestrictedBeforeCapabilityHook`. The registry installer would
construct one of these per `[[hooks]]` entry whose body is
`HookManifestBody::Predicate`.
* Sink reasons are `&'static str`, so the dynamic predicate `reason`
surfaces in audit (via the evaluator's `EvaluatorDecision`) rather
than the model-visible decision. Closed-vocabulary labels carry
through to the sink.
4. Reborn composition seam (`ironclaw_reborn::loop_driver_host`)
* `RebornLoopDriverHostFactory::with_hook_dispatcher(Arc<HookDispatcher>)`
opt-in builder method. When set, the factory wraps the capability
and prompt ports with the hooked middleware. Default behavior
(no dispatcher) is unchanged from the pre-hooks shape, so existing
callers continue to work.
* Added `ironclaw_hooks` as a dep in `ironclaw_reborn`.
Test plan
=========
* `cargo test -p ironclaw_hooks` — 60 tests pass (59 unit + 1
integration smoke; +13 vs the foundation commit covering middleware,
evaluator, installed_hook).
* `cargo test -p ironclaw_reborn` — 118 tests pass; no regressions
from adding the dep.
* `cargo test -p ironclaw_architecture` — 13 tests pass; the
`ironclaw_turns -> ironclaw_hooks` boundary still holds and the new
`ironclaw_hooks` rule (no host_runtime / dispatcher / secrets /
network / wasm / reborn) is unaffected.
* `cargo clippy -p ironclaw_hooks --all-targets --all-features
-- -D warnings` — clean.
* `cargo clippy -p ironclaw_reborn --all-targets -- -D warnings` —
clean.
* `cargo fmt --all -- --check` — clean.
What still defers
==================
* WASM hook execution path.
* Persistent predicate counter (in-memory only for now).
* Argument-extraction so `NumericSum` predicates evaluate against
capability arguments.
* Gate-ref plumbing so PauseApproval / PauseAuth surface real
`CapabilityOutcome::ApprovalRequired` instead of `Denied`.
* Prompt-snippet injection (waits for shared envelope helper).
* Event-triggered hooks.
* Self-authored hooks (#3567).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…bserver middleware Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…stFactory
Adds crates/ironclaw_reborn/tests/hooks_integration.rs covering the
factory's HookDispatcher wiring seam end-to-end. Tests drive
host.invoke_capability(...) (not dispatcher.dispatch_before_capability(...)
directly) so a regression in RebornLoopDriverHostFactory's wrapping
composition surfaces here.
Scenarios:
- PredicateBackedBeforeCapabilityHook (DenyCapability NameEquals
"cap.blocked") short-circuits invocation; inner port never called;
outcome is Denied(unknown("hook_denied")).
- A privileged selective hook that allows non-matching capabilities
proves the wrapper does not blanket-deny: cap.allowed reaches the
inner port and completes once.
- Factory built without with_hook_dispatcher() lets cap.blocked through
to the inner port, proving the hook plumbing is genuinely opt-in.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lding
Three additions to ironclaw_hooks:
B. `pass()` on gate sinks — distinguishes "evaluated, no opinion" from
"returned without minting a decision." A passing hook contributes
nothing to the composed decision; a silent hook is still Malformed
and fails closed. `PredicateBackedBeforeCapabilityHook` now routes
the evaluator's `Allow` decision through `sink.pass()` instead of
the previous `deny("hook_predicate_pass")` workaround.
A. `HookRegistrar` bridge — converts a `Vec<HookManifestEntry>` into
`HookBinding`s + dispatcher impls in one call. Predicate bodies are
wired through `PredicateBackedBeforeCapabilityHook`; WASM bodies
return `HookError::RegistryConstruction` for now. Adds
`HookDispatcher::insert_binding` so the registrar can mutate the
registry through the dispatcher rather than reach inside.
I. Self-authored hooks scaffolding — fourth `HookTrustClass` variant
for hooks the agent authors at runtime. Run-scoped only;
monotonic-restriction sink with no `allow`, no trusted-snippet path,
no effect-class constructor. Closed-vocabulary `SelfAuthoredReason`
enum keeps free-text reasons off the audit seam.
`SelfAuthorshipProvenance` captures authoring run/turn, timestamp,
spec digest, optional user ratification, and a generation-trace
pointer. Durable persistence depends on the unforgeable channel
from #3564 and lands separately.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Port observer middleware
…nLoopDriverHostFactory
… decisions
Previously, `GateDecisionInner::PauseApproval` and `PauseAuth` returned by
hooks were degraded to `CapabilityOutcome::Denied` at the middleware
boundary because the hook crate had no way to mint a `LoopGateRef` scoped
to the current run. Hooks that wanted to pause the loop for approval or
auth instead failed the call closed, leaving the host's approval-router
machinery unreachable from hook code.
This change introduces a `HookGateRefFactory` trait in
`ironclaw_hooks::middleware::gate_ref` that mints `LoopGateRef`s for
pause-class decisions. `HookedLoopCapabilityPort` now takes an
`Arc<dyn HookGateRefFactory>`, defaulting to `UuidHookGateRefFactory` (a
locally-unique opaque-id factory suitable for tests and the foundation
slice). Production deployments override via `.with_gate_ref_factory(...)`
with a factory bound to the current `LoopRunContext` and the host's
gate-router.
The translation in `decision_to_outcome` is now async so it can await the
factory. `PauseApproval` maps to `CapabilityOutcome::ApprovalRequired
{ gate_ref, safe_summary }` and `PauseAuth` to `AuthRequired`. If the
factory itself errors, the middleware falls back to `Denied` with a
sanitized `hook_gate_ref_unavailable` reason kind so the loop fails
closed rather than routing through an unresolvable suspension. The
underlying error text is dropped to avoid leaking gate-router state into
model-visible output.
Tests:
- `pause_approval_decision_surfaces_as_approval_required`,
`pause_auth_decision_surfaces_as_auth_required`,
`gate_ref_factory_failure_falls_back_to_denied` in
`middleware::capability_port::tests`.
- `pause_approval_hook_surfaces_as_approval_required_with_real_gate_ref`
in `crates/ironclaw_reborn/tests/hooks_integration.rs`, exercising the
full `RebornLoopDriverHostFactory` composition with the default
`UuidHookGateRefFactory`.
- Gate-ref factory unit tests in `gate_ref::tests`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ument extraction Wires the missing argument-extraction story for the predicate evaluator so `ValueOrRateBound::NumericSum` actually enforces a rolling numeric cap instead of warn-and-allowing. - Extend `BeforeCapabilityHookContext` with a sealed `SanitizedArguments` view. Strings truncate to 256 bytes; objects/arrays cap at 8-deep. `extract_numeric` supports dotted + bracketed paths (`order.amount`, `items[0].price`) and returns `Option<rust_decimal::Decimal>`. The inner representation is sealed so external callers can't bypass bounds. - Introduce `CapabilityInputResolver` + bundled `NullCapabilityInputResolver` in `middleware/resolver.rs`. The hooks crate intentionally doesn't know how to dereference a `CapabilityInputRef` — that knowledge belongs to the production host. Until a real resolver is wired in (follow-up), arguments are `Unresolved` and `NumericSum` fails closed. - `HookedLoopCapabilityPort::new` defaults to the null resolver; new builder `.with_resolver(Arc<dyn CapabilityInputResolver>)` overrides. - `PredicateEvaluator` gains a tenant-keyed `value_history` map. The `NumericSum` arm parses `max` + `window`, extracts the numeric value from sanitized args, accumulates within the rolling window, and applies `on_exceeded` when the sum exceeds the cap. Unresolved args, missing field, non-numeric field, unparseable max, and unparseable window all fail closed via the configured `OnExceededAction`. - Add `BeforeCapabilityHookContext::new_unresolved(...)` convenience ctor; existing test sites switch to it instead of churning every call site through the 4-arg ctor. Test count: +14 (8 new SanitizedArguments tests, 6 new NumericSum evaluator tests, 1 null-resolver test; one old NumericSum-stub-related gap closed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ning Addresses blocking findings from the security audit of `ironclaw_hooks`: - C1 (Blocking, Trust Model): "Installed cannot Allow" was not enforced at the registration boundary. `BeforeCapabilityHookImpl::Privileged` was a public variant, so external crates with dispatcher access could construct an Installed binding paired with a Privileged impl and bypass the sink trait restriction. Sealed `BeforeCapabilityHookImpl`, `BeforePromptHookImpl`, and `ObserverHookImpl` to `pub(crate)` and replaced the single generic `install_before_capability` / `install_before_prompt` / `install_observer` surface with tier-specific public installers (`install_builtin_*`, `install_trusted_*`, `install_installed_*`) that build the binding with the matching trust class internally. Updated registrar, internal middleware tests, the hooks foundation pipeline test, and the reborn `hooks_integration` test to drive the new surface. Added regression tests proving the trust class is set by the installer and that the seal is type-level. - C5 (Medium, Slot Poisoning): same-dispatch poisoning was incomplete because `ordered_bindings` snapshots once at the top of the loop, and `HookRegistry::insert` accepted duplicate hook IDs. Rejected duplicate hook IDs (any point) in `HookRegistry::insert` and added a poison re-check before invoking each hook impl in `dispatch_before_capability`, `dispatch_before_prompt`, and `dispatch_observer_at`. Added regression tests for both behaviors. - C6 (Medium, Manifest / Predicate Validation): `parse_window` could panic on non-ASCII input because `split_at(len - 1)` requires a char boundary. Rewrote to compute the unit char's UTF-8 byte length and slice safely, added a public `validate_window` helper, and wired it into `HookManifestEntry::validate` for both `InvocationCount` and `NumericSum` bounds. Added tests for non-ASCII, empty, single-char, and zero-duration windows. - C2 (High, Tenant Isolation): partial fix only. The `PredicateEvaluator`'s sliding-window counter was keyed by `(hook_id, capability)`, so cross-tenant state could leak. Extended `HistoryKey` to include `tenant_id` and added a regression test proving counters partition by tenant. Documented the broader dispatcher-per-build / per-run-fresh-dispatcher pattern as deferred follow-up in `crates/ironclaw_hooks/CLAUDE.md`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wires the hook dispatcher into the host's milestone stream so audit backends and SSE observers can see hook activity. Previously, hook dispatch was invisible — denies, pauses, failures, and observer fires left no trace in the host's observability backend. Changes: - `ironclaw_turns`: add `HookDispatched`, `HookDecisionEmitted`, and `HookFailed` variants to `LoopHostMilestoneKind`, with a closed- vocabulary `HookDecisionSummary` enum (Allow/Deny/PauseApproval/ PauseAuth/Pass/Patch). Introduce a lightweight `HookMilestoneSink` trait that emits hook-specific *kinds* without requiring a `LoopRunContext` (the dispatcher is a process-wide singleton that cannot own a per-run context), plus a `RunScopedHookMilestoneSink` adapter that injects run context and forwards to the existing `LoopHostMilestoneSink`. Also add `InMemoryHookMilestoneSink` for tests. - `ironclaw_hooks`: add a `telemetry` module that converts hook-crate types (`HookId`, `HookTrustClass`, `HookPointSpec`, `FailureCategory`, `FailureDisposition`, `BeforeCapabilityHookDecision`) into the wire- shape labels and summaries the milestone sink expects. Hook ids cross the seam as hex strings because the strongly-typed `HookId` cannot be imported from `ironclaw_turns` (the architecture test enforces `ironclaw_turns -> ironclaw_hooks` stays absent). - `ironclaw_hooks::dispatch`: add an optional `Arc<dyn HookMilestoneSink>` to `HookDispatcher`, set via `with_milestone_sink`. Emit `HookDispatched` before each hook runs, `HookDecisionEmitted` after a decision/pass/patch, and `HookFailed` on timeout/panic/malformed/missing-impl across all three dispatch paths (before_capability, before_prompt, observer). Default behavior (no sink attached) emits nothing — preserves the pre-telemetry observable surface. - `ironclaw_reborn`: document on `with_hook_dispatcher` that callers attach the milestone sink to the dispatcher *before* wrapping it in `Arc` and installing it into the factory, using a `RunScopedHookMilestoneSink` to inject run-context. The dispatcher itself is shared across runs, so attaching a fixed run-context inside it would be wrong. Update `RuntimeEvent` projection in `milestone_events.rs` to ignore the new hook kinds (no projection pathway yet; emitted milestones are consumed by SSE observers directly). Tests: - `ironclaw_hooks::dispatch`: 5 new tests covering milestone emission for deny decisions, panic failures, prompt-mutator patches, observer pass-throughs, and the no-sink default. - `ironclaw_reborn` hooks_integration: end-to-end test wiring a `RunScopedHookMilestoneSink` onto the dispatcher and asserting hook activity surfaces in the host's `LoopHostMilestoneSink`. Total: +6 hook telemetry tests; no existing tests modified. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…o prompt bundle Adds `ironclaw_prompt_envelope`, a leaf crate that owns the single envelope primitive used by every model-visible untrusted-content path. `wrap_untrusted` prefixes content with a closed-vocabulary `<Trusted|Untrusted> <source> content: ` marker, rejects bodies carrying instruction-hijack phrases (`ignore previous instructions`, `<|im_start|>`, `<system>`, etc.), and enforces a 4 KiB byte budget by default. Migrates `ironclaw_host_runtime::memory_context` to delegate envelope wrapping, marker rejection, and control-character stripping to the new crate while keeping the `LoopSafeSummary`-specific 512-byte cap and byte truncation local. Existing memory_context behavior and tests are preserved. Wires the same envelope into `ironclaw_hooks`: * `HookPatch::add_enveloped_snippet` now takes a raw body and wraps it via `wrap_untrusted(EnvelopeSource::Hook, …)`. `Installed` hooks produce `Untrusted` envelopes; `Builtin`/`Trusted`/`SelfAuthored` produce `Trusted` envelopes so downstream readers can distinguish the two paths through a uniform marker. * `HookedLoopPromptPort::build_prompt_bundle` is no longer observe-only. After dispatching `before_prompt`, it envelope-wraps every snippet patch (passing `Enveloped` through, wrapping `Trusted` with the envelope helper), enforces the 4 KiB aggregate snippet byte budget across patches, and appends the wrapped snippets to the prompt bundle's `messages` as `system`-role `LoopModelMessage` entries carrying deterministic `msg:hook.<ordinal>.<hash>` content refs (mirroring the skill-snippet ref convention). The envelope crate is a leaf with no ironclaw dependencies, satisfying the boundary contract; the existing `ironclaw_hooks` boundary rule in `reborn_dependency_boundaries` continues to hold because `ironclaw_prompt_envelope` is not on its forbidden list. Test count delta: * `ironclaw_prompt_envelope`: +13 new tests (crate did not exist). * `ironclaw_hooks`: 84 → 88 tests (+4 prompt-port behavior tests: `hook_patch_appended_as_envelope_wrapped_message`, `total_byte_budget_enforced_across_patches`, `instruction_hijack_in_patch_rejected`, `trusted_hook_patch_wrapped_with_trust_marker`). * `ironclaw_host_runtime` memory_context: unchanged (8 tests still pass). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eAuth # Conflicts: # crates/ironclaw_hooks/src/middleware/capability_port.rs # crates/ironclaw_reborn/tests/hooks_integration.rs
# Conflicts: # crates/ironclaw_hooks/src/dispatch.rs
Add a "Loader responsibility" section to ironclaw_hooks/CLAUDE.md explaining that tier-specific installers prevent minting wrong-tier impls but cannot enforce origin — that's the loader's job — and recommending registry loaders type-tag extension hooks as LoadedHook::Installed at the loader seam. Add tier_specific_installers_are_documented_as_loader_contract as a regression guard that touches every public install_*_before_capability and install_*_before_prompt method so any signature change forces the loader contract to be re-evaluated. Document HookId::to_hex's 64-char lowercase hex output as part of the cross-crate contract consumed by LoopHostMilestoneKind::Hook* in ironclaw_turns; add hook_id_hex_format_is_stable_64_lowercase_chars in identity::tests and hook_id_string_serialization_matches_to_hex in telemetry::tests to pin the format and the seam conversion path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add L3 schema-snapshot tests for every hook-related LoopHostMilestoneKind variant (HookDispatched, HookDecisionEmitted per HookDecisionSummary, HookFailed per FailureCategory) so downstream consumers can rely on the JSON wire shape and any accidental field rename, enum-tag rename, or type change fails loudly. Add L4 pairing-invariant matrix test in the hook dispatcher that drives every observable outcome (Allow, Deny, PauseApproval, PauseAuth, Pass, Panic, Timeout, Malformed, MissingImpl) through a recording milestone sink and asserts the dispatched-then-terminator pairing shape. Document the MissingImpl path as the one case that emits a sole HookFailed with no preceding HookDispatched (the dispatcher discovers the protocol violation before the hook is actually dispatched). Add a multi-hook dispatch test that installs three hooks with mixed outcomes (allow/deny/panic) at the same point and asserts each hook produces its own paired sequence in the deterministic (phase, priority, hook_id) order taken from the dispatcher's registry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nLoopDriverHostFactory Wire the HookedLoopModelPort / HookedLoopTranscriptPort / HookedLoopCheckpointPort observer wrappers into RebornLoopDriverHostFactory::build_text_only_host_with_capabilities, mirroring the existing HookedLoopCapabilityPort / HookedLoopPromptPort composition. The wrappers are applied only when a HookDispatcher is set on the factory, so the default factory shape is unchanged. Add four integration scenarios in crates/ironclaw_reborn/tests/hooks_integration.rs: - observer_hook_fires_after_model_through_factory - observer_hook_fires_after_capability_through_factory - observer_hook_fires_after_checkpoint_through_factory - observer_panic_does_not_fail_model_call (panic-isolation regression) Relax the test-fixture model gateway from "panic if invoked" to returning a stub assistant reply so the AfterModel / panic-isolation tests can drive stream_model through the wrapped port. The existing capability-port tests never touch the gateway, so their behavior is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ink wiring Adds a `HookDispatcherBuilder` in `ironclaw_hooks::dispatch` that owns the dispatcher construction lifecycle: registry -> optional timeout -> optional milestone sink -> installed hooks -> `.build_arc()`. The terminal `.build_arc()` wraps in `Arc` and yields an immutable handle. Tightens the public surface on `HookDispatcher`: `new`, `with_timeout`, `with_milestone_sink`, and every `install_*_*` method are now `pub(crate)`. Outside callers route exclusively through the builder, so "wire the milestone sink before Arc-wrapping" is a compile-time fact rather than a documentation convention. `HookRegistrar::install` now takes a `HookDispatcherBuilder` by value and returns `(HookDispatcherBuilder, Vec<HookId>)`, keeping the builder chainable through manifest installation. `RebornLoopDriverHostFactory` gains `with_hook_dispatcher_builder` to let callers defer `.build_arc()` to the factory — a step toward the FU8 per-build dispatcher pattern. Migrates `foundation_pipeline.rs` and `hooks_integration.rs` to the builder. Internal middleware and dispatch tests continue to use the crate-private `HookDispatcher::new` directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zmanian
added a commit
that referenced
this pull request
May 23, 2026
…D5 regression from r3) Round 3 of PR #3573 (already merged into hooks-foundation-01) added an inline per-key sample cap of 4_096 in evaluator.rs to bound memory under attacker-triggered hot capabilities with very large declared windows (threat-model finding D5). The predicate-state extraction in PR #3635 moved that bookkeeping into the PredicateStateBackend trait but missed porting the cap, so the cap would silently disappear from production once this PR rebases onto the foundation branch. This commit moves the cap into the in-memory backend impl next to MAX_HISTORY_KEYS / MAX_KEYS_PER_TENANT and enforces it in both record_invocation and record_value. For the NumericSum path, the bucket helper's pop_front already decrements running_sum, so the incremental sum invariant survives cap-driven eviction. The pre-existing inline copy in evaluator.rs becomes redundant once the trait impl owns the enforcement; the rebase resolution deletes it. Adds two regression tests: - record_invocation_caps_samples_per_key_under_attacker_pressure - record_value_evicts_oldest_keeping_running_sum_consistent
zmanian
added a commit
that referenced
this pull request
May 23, 2026
* Implement installed WASM hook runtime Adds crates/ironclaw_hooks/docs/threat-model-wasm.md and follows the reviewed design ack: 1) module bytes are resolved, digest-cached, and compiled in the tool-WASM style while reusing its resource limiter; 2) each invocation gets a fresh wasmtime Store; 3) the ABI is a wasmtime::Linker surface, not wit-bindgen; 4) host-import sink shims enforce call, patch-byte, observer-fact, and decision budgets. * Harden WASM hook string and metadata budgets * fix(hooks): validate WASM hook ABI at install time (serrrfirat #3 on PR #3634) Address serrrfirat MEDIUM finding #3: `WasmHookRuntime::prepare()` compiled and cached module bytes but did not validate imports or the requested export. ABI mismatches (unsupported import, missing export, wrong export signature) were deferred to first live dispatch — and the prior `wasm_unsupported_host_import_fails_closed` test codified that a bad-import module would install successfully and only fail closed at invocation. Malformed untrusted modules should never reach live traffic. Changes: - `prepare()` derives the target hook point from `request.kind`, then runs `validate_module_abi()`: scratch-instantiate the module against the point-specific linker (catches unsupported / wrong-type imports) and resolve the typed export `() -> ()` (catches missing export and wrong signature). Failures surface as new `WasmHookRuntimeError::InvalidImports` or existing `WasmHookRuntimeError::InvalidExport`, both of which bubble up as `HookError::RegistryConstruction` from the registrar. - `wasm_point_for_kind(HookManifestKind)` helper centralizes the kind → wasm-point mapping; the previous `execute_*` paths can share it in a follow-up but kept inline for now to minimize churn. Tests: - `wasm_unsupported_host_import_is_rejected_at_install_time`: replaces the prior test that codified late-failure behavior; asserts the registrar returns `RegistryConstruction` citing the bad import. - `wasm_missing_export_is_rejected_at_install_time`: new module that compiles but lacks the manifest-declared export; same install-time rejection. * fix(hooks): address henrypark133 must-fix #1, #2, #3 on PR #3634 Three items from the 5-15 review: **#1 (must-fix) Extract ironclaw_wasm_limiter micro-crate** Replace `#[path = "../../../ironclaw_wasm/src/limiter.rs"]` cross-crate file import with a proper Cargo edge. The 111-line `WasmResourceLimiter` moves into a new `crates/ironclaw_wasm_limiter` micro-crate that both `ironclaw_wasm` and `ironclaw_hooks` depend on. The architecture rule forbidding `ironclaw_hooks -> ironclaw_wasm` is preserved (the new crate sits below both consumers and pulls in only `wasmtime` + `tracing`); `cargo check`, `cargo doc`, and architecture-linting tests now see the edge, and the file can't be moved out from under one of the consumers silently. Mechanical changes: - new `crates/ironclaw_wasm_limiter/` (Cargo.toml + src/lib.rs with the type exposed as `pub` instead of `pub(crate)`) - workspace `members` entry added - `crates/ironclaw_wasm/src/limiter.rs` deleted - `crates/ironclaw_wasm/src/lib.rs`: `mod limiter` removed - `crates/ironclaw_wasm/src/store.rs`: import switched to `ironclaw_wasm_limiter::WasmResourceLimiter` - `crates/ironclaw_wasm/Cargo.toml`: dep added - `crates/ironclaw_hooks/Cargo.toml`: dep added - `crates/ironclaw_hooks/src/wasm/runtime.rs`: `#[path = ...]` block removed; import switched to the crate **#2 + #3 (must-fix) Dead WASM arms in dispatch** `run_before_capability_hook`, `run_before_prompt_hook`, and `run_observer_hook` each had an early-return guard that dispatched WASM hooks with `catch_unwind` + timeout, then ALSO had a matching WASM arm in the inner `match` that ran without those protections. The prompt-path arm additionally swallowed `WasmHookFailure` via `|_| ()`, making the must-fix #2 problem worse on that path specifically. If a future refactor removed any of the early-return guards, those inner arms would silently take over and drop panic isolation, deadline enforcement, AND (for prompts) the failure category. Replaced each inner arm with `unreachable!()` carrying a comment that explains why the arm exists and references the early-return guard above it. A future refactor that removes the guard will now trip the `unreachable!` at first call instead of silently degrading. All 154 hooks lib + 29 reborn integration tests still pass. * fix(hooks): plumb context to WASM hooks + runtime hardening Critical #1 on PR #3634: WASM hooks previously received no context. The `execute_*` entry points dropped the `&BeforeCapabilityHookContext` / `&BeforePromptHookContext` / `&ObserverHookContext` value and invoked the guest export with `()`, so a WASM gate could never decide based on the capability name, tenant, provider, or other dispatch-time facts. Add an `ic:hooks/context@1` host-import module exposing two read-only calls — `ctx_size() -> i32` and `ctx_read(ptr, len) -> i32` — backed by a JSON-serialized blob the dispatcher writes per-invocation into the fresh store. Modules that don't import these continue to link; modules that do import them get a stable, non-empty payload to read. An integration test (`wasm_before_capability_hook_reads_context_blob`) asserts the contract end-to-end: a guest that fails to read a non-empty blob traps before its `deny` call. Also rolls up the other reviewer-flagged WASM runtime issues, all of which touch `wasm/runtime.rs`: HIGH #2: epoch-tick background thread now holds a shutdown `AtomicBool` and joins on `Drop`. Previously it looped forever and leaked an Engine clone on every runtime drop. MED #4: compiled-module cache is now an `lru::LruCache` bounded by `MODULE_CACHE_CAPACITY = 128`. Replaces the unbounded `HashMap`. MED #7: `prepare()` no longer compiles under the cache lock. Fast path reads from LRU under a brief lock; slow path compiles outside the lock and re-checks on insert to avoid the TOCTOU window where two concurrent installs of the same module both compile. Bug #9: post-call `deadline_exceeded()` re-check on the Ok branch is gone. wasmtime epoch-interrupt is the authoritative wall-clock signal; an Ok return is no longer reclassified as a timeout because the wall ticked over during host-side return. Bug #10: `add_milestone_metadata` returns a distinct "metadata value exceeds the u32 byte-length ceiling" error when the guest-supplied `value.len()` overflows u32, instead of misreporting it as "exceeded total prompt-patch byte budget". Existing integration tests for WASM hooks are also re-wired through `HookRegistrar::with_verified_grants` so the grants-store gate added in the foundation-01 merge stops failing the pre-existing fixtures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): run WASM hooks on the blocking pool HIGH #3 on PR #3634: `tokio::time::timeout` does NOT cancel synchronous wasmtime execution. The previous code awaited a `catch_unwind(async { h.evaluate(ctx) })` future whose body completed in one poll, so the timeout could only fire *around* the WASM call rather than against it; a hook that wedged inside wasmtime simply pinned the calling tokio task. Route gate, prompt, and observer WASM dispatch paths through `tokio::task::spawn_blocking` via a shared `run_wasm_blocking` helper. The outer `tokio::time::timeout` now governs the JoinHandle, so a stuck blocking task stops blocking the dispatcher's caller; the wasmtime epoch interrupt configured in the runtime (10 ms tick) is the authoritative in-WASM wall-clock cancel signal. JoinError (panic in the blocking task) maps to `FailureCategory::Panic`, matching the pre-existing semantics for synchronous panics caught via `catch_unwind`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * perf(hooks): O(1) hook-id lookup via side index Finding #8 on PR #3634: `set_priority`, `poison`, `is_poisoned`, and `contains_hook` all did full-registry scans over every binding at every point. Each is called per-dispatch (poison-checks on the snapshot loop in particular), so the cost is `O(registered_hooks)` per `(installed_hook, registered_hook)` pair. Maintain a denormalized `HashMap<HookId, (HookPointSpec, usize)>` side index in lock-step with `by_point` so every per-hook-id operation becomes a single hash lookup + a direct vec indexed access. The duplicate-id rejection in `insert` now reads from the side index too, turning what used to be a flat-map scan into a `HashMap::contains_key`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(hooks): wall-clock timeout, observer memory, limiter rollback, registrar happy path Round out the test set for the WASM hook execution path: #11 / #12: gate + observer wall-clock timeout. The pre-fix dispatcher ran wasmtime synchronously on the executor, so the outer `tokio::time::timeout` `Err(_elapsed)` arm was effectively unreachable. Now that WASM execution runs on the blocking pool, the timeout actually fires; the new tests give the wasm budget headroom (1B fuel, 5s wall) and the dispatcher a 20 ms timeout, then assert the failure classification (FailClosed for gate, FailIsolated for observer). #13: observer memory exhaustion. Mirrors `wasm_memory_exhaustion_fails_closed_for_gate` against the observer dispatch path so the FailIsolated branch of the failure matrix has explicit memory coverage, not just fuel/wall. #15: `WasmResourceLimiter::memory_grow_failed` rollback. Stages an approved grow, simulates the OS-level grow failing, and asserts a subsequent grow of the full ceiling succeeds — the inflated `memory_used` from the failed attempt must be released. #16: registrar WASM happy path. Companion to the existing `install_wasm_body_requires_runtime` negative case: a valid module installs, the binding is visible via the public registry accessor, and is not pre-poisoned. #14 (`add_milestone_metadata` happy path) is intentionally omitted — the BeforePrompt dispatch path is currently unreachable due to a pre-existing manifest-vs-registry scope conflict (`OwnCapabilities` is the only valid `BeforePrompt` scope per manifest validation, but the registry rejects `OwnCapabilities` at `BeforePrompt` because the point has no provider context). That contradiction sits outside this PR's scope; flagging for a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(hooks): typed WASM version material, reconcile design doc LOW #20 on PR #3634: extract the `{extension_version}+wasm:{module_digest_hex}` concatenation into a `WasmVersionMaterial` newtype with a single `Display` impl. The identity material no longer floats free as a stringly-typed argument inside the registrar. Reconcile `docs/successors/02-wasm-runtime.md` with the implementation: - Spell out that wall-clock cancellation depends on the `tokio::time::timeout(tokio::task::spawn_blocking(...))` pair, and explain why a bare timeout over a synchronous wasmtime call cannot actually cancel. - Define `FailIsolated` and `FailClosed` as `FailureDisposition` values, distinct from the older `HookFailureMode::{FailOpen, FailClosed}` policy switch that applies to predicates. - Clarify the generic `evaluate` export contract — name is whatever the manifest declares, signature is `(): ()`, context arrives through the new `ic:hooks/context@1` host imports — and note the intentional divergence from `WitToolRuntime`'s hardcoded interface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): drop .expect() in WASM module cache capacity Pre-commit no-panics CI flagged the .expect() on the LruCache capacity. Move the validity check to a const match, so the NonZeroUsize is fixed at compile time and the no-panics regex is satisfied. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): use HookLocalId::new after newtype privatization The newtype-privatization landed in reborn-integration after the hooks-fu-wasm-runtime branch's WASM scaffolding tests were written; update the affected test/registrar sites to use HookLocalId::new instead of the now-private tuple constructor. * style: cargo fmt after newtype-privatization fixups * test(hooks): ignore 3 BeforePrompt WASM tests with manifest/registry conflict These tests were failing on the original branch tip too (verified against origin/hooks-fu-wasm-runtime @ 571efdf). The Installed-tier BeforePrompt WASM install path has no valid scope today: - OwnCapabilities is rejected by the registry C3 check (finding #2 on PR #3573) since BeforePrompt has no per-capability invocation context. - SameTenant is rejected by manifest validation ("cannot combine scope = same_tenant with kind = before_prompt"). The budget-overflow paths these tests exercise are point-agnostic; the follow-up is to either rewrite the helper to install through BeforeCapability or add a Global manifest scope. Tracked as a deferred item on the new PR. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zmanian
added a commit
that referenced
this pull request
May 23, 2026
Successor PR from #3573. Adds a new EventTriggered hook point that subscribes to RuntimeEvents asynchronously, outside the loop's inline tick. Observer-only by construction (no Allow/Deny/Patch); typed against a narrowed HookObservableEvent projection to keep the cross-crate boundary clean. Scope doc only; design questions about cursor/replay semantics and per-extension event-rate caps need design review before implementation.
zmanian
added a commit
that referenced
this pull request
May 23, 2026
…reality Address gemini-code-assist review on `04-event-triggered-hooks.md`: - L50 (Likely surface): annotated the sketch's full `RuntimeEvent` use with a pointer to the narrowed-projection follow-up so the snippet no longer reads as a recommendation contradicting L119–121. - L55 (sink methods): replaced `note_fact` / `emit_audit` (which never shipped on `ObserverSink`) with the actual `note(category, summary)` primitive and cross-referenced Reborn's `EventTriggeredObserverSink`. - L95 (cursor / replay): "lost events during downtime acceptable" contradicted the at-least-once replay semantics described in the Phase 5 implementation notes. Rewrote the bullet to say replay is at-least-once from the persisted cursor and to spell out the operator obligation around cursor persistence before shutdown. - L100/115 (forbids events dep): the original doc claimed `ironclaw_hooks` forbids an `ironclaw_events` dep, but the Risk section noted the dep is already established via PR #3573. Updated both passages to reflect that the dep direction is set; Phase 5 adds the *consumer* side. The narrowed `HookObservableEvent` projection is now framed as a follow-up tracked in #3690.
zmanian
added a commit
that referenced
this pull request
May 23, 2026
zmanian
added a commit
that referenced
this pull request
May 23, 2026
Address serrrfirat's #3 follow-up from PR #3573 — partial fix promoted to full pin. Two new tests in capability_port.rs::tests: - invocation_arguments_digest_is_stable_for_known_inputs: pins the digest for a fixed (capability_id, input_ref) fixture so any future change to the hashing structure or input-ref format is loud. The captured hex is documented in the assertion message + stability contract. - invocation_arguments_digest_differs_for_different_input_refs: structural sanity check that distinct inputs produce distinct digests. Plus expanded rustdoc on the function calling out the stability contract: changing the hashing path requires updating the fixture, surfacing in the cross-crate wire-format contract section, and bumping the framework's contract version if downstream consumers exist. Tests: 156 unit tests pass (was 154; +2 new). Clippy + fmt clean.
zmanian
added a commit
that referenced
this pull request
May 23, 2026
Successor PR from #3573. Adds a new EventTriggered hook point that subscribes to RuntimeEvents asynchronously, outside the loop's inline tick. Observer-only by construction (no Allow/Deny/Patch); typed against a narrowed HookObservableEvent projection to keep the cross-crate boundary clean. Scope doc only; design questions about cursor/replay semantics and per-extension event-rate caps need design review before implementation.
zmanian
added a commit
that referenced
this pull request
May 23, 2026
…reality Address gemini-code-assist review on `04-event-triggered-hooks.md`: - L50 (Likely surface): annotated the sketch's full `RuntimeEvent` use with a pointer to the narrowed-projection follow-up so the snippet no longer reads as a recommendation contradicting L119–121. - L55 (sink methods): replaced `note_fact` / `emit_audit` (which never shipped on `ObserverSink`) with the actual `note(category, summary)` primitive and cross-referenced Reborn's `EventTriggeredObserverSink`. - L95 (cursor / replay): "lost events during downtime acceptable" contradicted the at-least-once replay semantics described in the Phase 5 implementation notes. Rewrote the bullet to say replay is at-least-once from the persisted cursor and to spell out the operator obligation around cursor persistence before shutdown. - L100/115 (forbids events dep): the original doc claimed `ironclaw_hooks` forbids an `ironclaw_events` dep, but the Risk section noted the dep is already established via PR #3573. Updated both passages to reflect that the dep direction is set; Phase 5 adds the *consumer* side. The narrowed `HookObservableEvent` projection is now framed as a follow-up tracked in #3690.
zmanian
added a commit
that referenced
this pull request
May 23, 2026
#3635) * docs(hooks): scope persistent predicate counter backend (successor #3) Successor PR from #3573. Current sliding-window state is in-memory and resets on restart. Adds a PredicateStateBackend trait + Postgres/libSQL impls for cross-process and restart-survival semantics. * feat(hooks): extract PredicateStateBackend trait + replay-safe in-memory impl Addresses codex review's three Critical findings on PR #3635: 1. Backend wiring: the trait is now registered (lib.rs:25-26) and PredicateEvaluator delegates to Arc<dyn PredicateStateBackend> via with_backend(...). Default constructor preserves the in-memory behavior so all 154 existing tests pass unchanged. 2. Atomic record-and-read: each record_invocation / record_value call performs the write AND returns the resulting in-window count/sum under a single mutex (in-memory) / transaction (durable backends). Splitting into separate record + read would let two hosts each see 'under cap' and both proceed, drifting past max. 3. Replay refusal: each record call carries a PredicateEventId. Re-emitting the same event_id is a no-op against the count. In-memory backend implements via a per-key bounded set (RECENT_EVENT_ID_CAP = 256); durable backends will use INSERT … ON CONFLICT DO NOTHING. Trait surface (predicate_state.rs): - PredicateEventId(String): opaque dedup key - PredicateBackendError: thiserror enum for fallible durable backends; in-memory backend never returns Err - PredicateStateBackend trait with Result return types - InMemoryPredicateStateBackend default impl - MAX_HISTORY_KEYS const re-exported via evaluator for back-compat Evaluator changes (evaluator.rs): - holds Arc<dyn PredicateStateBackend> (no more inline maps) - evictions_observed() reads through to backend - synth_event_id() generates per-call-unique ids via a process-local atomic counter so tests with identical (hook, ctx, now) still produce distinct ids - LRU helpers + HistoryKey/ValueHistoryKey types moved into predicate_state.rs (as InvocationKey/ValueKey) Tests: - 6 new predicate_state tests: - in_memory_invocation_counts_within_window - in_memory_invocation_trims_outside_window - in_memory_value_sums_within_window - in_memory_tenant_isolation (regression on threat-model C2) - in_memory_duplicate_event_id_is_a_noop_for_invocations - in_memory_duplicate_event_id_is_a_noop_for_values - 160 unit tests pass total. Reborn hooks_integration unchanged at 19 scenarios. Clippy/fmt/no-panics clean. Sync trait + Instant timestamps documented as a v1 choice; durable backends (Postgres, libSQL) will need an async companion trait using SystemTime — tracked in the scope doc as the next slice. Scope doc: crates/ironclaw_hooks/docs/successors/03-persistent-counter.md * fix(hooks): close codex P1 bugs in PredicateStateBackend in-memory impl Addresses codex P1 review on PR #3635: P1 #1 — replay dedup loss under high-throughput keys The prior design used a fixed-size (256) recent_ids ring per bucket decoupled from entries. Under any workload with >256 distinct events in the same window, the first event's id aged out of the ring while its timestamp entry was still live, so a replay silently re-counted. Fix: dedup memory is now intrinsic to entries. Each entry stores (timestamp, event_id), and the dedup check is 'does any in-window entry have this id?'. Dedup memory is therefore exactly the in-window entry set — no fixed cap, no silent loss. P1 #2 — zombie buckets clogging LRU Two-part fix: 1. record_* drops empty buckets eagerly via history.remove(key). This is mostly defense-in-depth — under the new dedup design, the record path can't actually leave a bucket empty (proved in the test rationale comment). 2. evict_lru_* now preferentially targets empty buckets first (find any v.entries.is_empty()), only falling back to the oldest-timestamp scan if no empty bucket exists. Filter-out behavior is gone, so any empty bucket that somehow survives becomes the next eviction victim instead of a permanent zombie. Test changes (+2 new, -0 removed): - dedup_memory_covers_full_window_under_high_throughput: pushes 512 distinct events into one bucket, then replays event-0. Pre-fix this would have counted again (silent dedup loss); post-fix the replay is a no-op. - lru_evicts_empty_buckets_first: crafts an empty bucket alongside a live one, runs LRU eviction, asserts the empty one is evicted and the live one retained. Tests: 162 unit total (+2 new). Clippy/fmt/no-panics clean. * docs(hooks): address gemini review on persistent-counter scope doc Four medium-priority doc nits from gemini-code-assist on the crates/ironclaw_hooks/docs/successors/03-persistent-counter.md scope: 1. run_id in the trait: removed. The trait dedupes on event_id (RuntimeEventId is already run-scoped), not run_id. Replaces the earlier 'backend stores (timestamp, run_id, event_id)' claim. 2. SystemTime vs chrono::DateTime<Utc>: switched to DateTime<Utc> to match project convention (src/db/mod.rs, ironclaw_events). The in-memory backend keeps Instant for monotonic process-local semantics; durable backends require DateTime<Utc> for cross- process serialization. Documented as a clock note. 3. libSQL TEXT column for rust_decimal: per src/db/CLAUDE.md, libSQL can't preserve Decimal precision with numeric/real types. LibSqlPredicateStateBackend serializes value as TEXT via Decimal::to_string() / from_str(). Postgres impl keeps numeric (correct for PG). Documented as the two LibSql-specific schema differences. 4. Batched-writes vs cross-process consistency tension: gemini was right that deferring writes to the tick boundary breaks requirement #1 (two hosts would each see 'under cap' simultaneously). v1 production backend keeps writes synchronous; future optimization batches reads (not writes). * fix(hooks): thread stable caller_event_id through hook context (replay dedup) henrypark133 HIGH on PR #3635 + serrrfirat HIGH #1: the `PredicateBackedBeforeCapabilityHook -> PredicateEvaluator` path always synthesized a fresh `event_id` per evaluation by mixing in a process-local atomic counter, so the same logical invocation retried/replayed always got a different id. The backend's UNIQUE constraint on `event_id` — the load-bearing dedup contract — never engaged on the real production path. Replay dedup was effectively "documented but unused." Plumb a stable per-invocation identity through the public hook surface: - `BeforeCapabilityHookContext` gains a `caller_event_id: Option<PredicateEventId>` field. Middleware that threads through from the calling layer's runtime event identity populates `Some(...)`; older / in-memory-only callers pass `None` and degrade to the current synth path (no behavior change). - New builder method `with_caller_event_id(...)`. - `PredicateEvaluator` resolves the id through a new `resolve_event_id` helper: prefer `ctx.caller_event_id`, fall back to `synth_event_id`. Both `record_invocation` and `record_value` paths use it. - Backend dedup behavior is unchanged — it was already correct on `event_id`. The bug was the caller path never supplying a stable id. Tests (caller-boundary, henrypark133's required regression): - `duplicate_caller_event_id_is_deduped_in_invocation_count`: two evaluations with the same `caller_event_id` count as one invocation; a third with a different id counts as two; a fourth crosses the cap. Sanity branch confirms the no-id synth path still exhibits "every call counts" semantics. This is the API contract slice. Wiring the middleware to actually supply a stable id (e.g. derived from the originating `RuntimeEventId` once that runs through the BeforeCapability path) is the follow-up that lights up the durable backend's end-to-end replay-safety promise. * refactor(hooks): demote PredicateStateBackend to pub(crate) (serrrfirat MED on PR #3635) serrrfirat MED: the `predicate_state` module exposed `PredicateStateBackend` as `pub`, but the trait's `now: Instant` parameter is process-local and not serializable. Any external durable backend impl built against the current trait would have to be rewritten when the durable contract lands with `chrono::DateTime<Utc>` (see successor doc 03-persistent-counter.md). Hold the public surface back until that contract is stable so we don't ship a public API we know we'll break. Demoted to `pub(crate)`: - `PredicateStateBackend` (trait) - `InvocationKey`, `ValueKey` (key types — backend ABI only) - `PredicateBackendError` (error type, with `#[allow(dead_code)]` on the `Unavailable` variant since the in-memory backend is infallible and no durable backend exists yet) - `InMemoryPredicateStateBackend` (the only impl) - `PredicateEvaluator::with_backend` (with `#[allow(dead_code)]` — reserved for future internal injection paths) Kept `pub`: - `PredicateEventId` — it appears on the public hook surface via `BeforeCapabilityHookContext::caller_event_id` (from the #3635 HIGH fix). Hook authors who want stable replay-dedup ids construct one. No behavior change. All 163 hooks lib tests + 19 reborn integration tests still pass. * fix(hooks): address henrypark133 must-fix #1-5 on PR #3635 Five items from the 5-15 review: **#1 (must-fix) O(n) dedup scan** The previous `bucket.entries.iter().any(...)` linear scan held the outer history mutex while walking thousands of in-window entries at high throughput. Add a companion `HashSet<PredicateEventId>` per bucket (`InvocationBucket.dedup_ids` / `ValueBucket.dedup_ids`), maintained alongside the deque via `pop_front`/`push_back` helpers. O(1) dedup, same correctness, same memory bound (one set entry per in-window entry — no fixed ring). **#2 (must-fix) Mutex poison cascade** `.expect("predicate history mutex poisoned")` propagated a panic to every subsequent caller. Replace with `match self.invocation_history.lock() { Ok(g) => g, Err(p) => p.into_inner() }` so a poisoning thread doesn't take down all subsequent evaluations. **#3 (must-fix) `caller_event_id` format validation** `with_caller_event_id` now rejects empty strings and ids containing NUL bytes. Failed validation logs a `tracing::warn!` and leaves `caller_event_id == None` so the synth path takes over — operator sees the warning, predicate dedup still works. Also: `PredicateEventId(pub String)` → `PredicateEventId(String)` with `new()` / `as_str()` (henrypark133 nit #9). Inner field is no longer in-place mutable from outside the crate. **#4 (must-fix) `with_backend` is `#[cfg(test)]`** Previously `#[allow(dead_code)]` — reachable from release builds and inviting future callers to inject backends through an unstable seam. Gated to `cfg(test)`. **#5 (important) `evict_older_than` trait stub** Default-impl no-op added to `PredicateStateBackend` so the trait signature is locked before the first durable-backend PR. Trait-object callers won't break when durable impls override it. **Bonus** (henrypark133 missing-coverage #1): `in_memory_record_invocation_is_atomic_under_concurrent_writers` — 32 threads each record a distinct event id; final count must equal 32, proving the atomic record-and-read contract holds under contention. **Bonus** (henrypark133 nit #10): The third stable id in `duplicate_caller_event_id_is_deduped_in_invocation_count` was 62 chars; bumped to 64 to match the synth output format. * fix(hooks): clippy doc-list-indentation + remove unused with_backend (#3635 CI) * fix(hooks): address serrrfirat HIGH + MEDIUM on PR #3635 (5-15 review) **MEDIUM — `caller_event_id` validation bypass** `with_caller_event_id` validated for empty/NUL but the field on `BeforeCapabilityHookContext` is `pub`, so callers could direct- assign `Some(PredicateEventId::new("..."))` with `new()` permissive and bypass the setter entirely. Move validation INTO the type boundary: - `PredicateEventId::new(...) -> Result<Self, PredicateEventIdError>` validates non-empty + NUL-free at construction. Any value that reaches a downstream backend now satisfies the format invariant by construction. - `PredicateEventId::new_unchecked(...)` for internal synth paths and tests that mint ids from known-good shapes (hex digests). - `with_caller_event_id` drops its now-redundant runtime check; the type already enforces it. - Internal synth in `evaluator.rs` switches to `new_unchecked` (64-char hex output is always valid by construction). Tests: - `predicate_event_id_rejects_empty` - `predicate_event_id_rejects_nul_bytes` - `predicate_event_id_accepts_typical_hex_digest` **HIGH — durable schema: dedup scope mismatch** The successor doc's Postgres schema declared `event_id uuid PRIMARY KEY` (globally unique), but the trait's replay-refusal contract dedupes within the counter `key`. `caller_event_id` is per capability invocation — two predicate-backed hooks observing the same invocation share an id. A global PK lets the first hook's INSERT win and silently undercounts the second hook's bucket. - `docs/successors/03-persistent-counter.md`: PK changes to composite `(tenant_id, hook_id, capability, event_id)` for invocations and `(tenant_id, hook_id, capability, field, event_id)` for values, matching the trait's per-key dedup scope. - `predicate_state.rs` trait doc: replay-refusal section rewritten to spell out the per-key scope and the corresponding `INSERT … ON CONFLICT (tenant, hook, capability[, field], event_id) DO NOTHING` shape durable backends should use. * docs(hooks): document host-assigned trust boundary on PredicateEventId henrypark133 / serrrfirat blocker B4 on PR #3635: the `caller_event_id` threading through `BeforeCapabilityHookContext` partially shipped earlier (commit b4d8a35), but the trust-boundary documentation explaining the host-assigned invariant was still missing. Add rustdoc to `PredicateEventId` and the `PredicateStateBackend` trait clarifying that: - the id MUST be minted by trusted host code from authoritative sources (dispatcher RuntimeEventId, host-side hash, arguments digest) - it MUST NOT pass through unchanged from any tenant-controlled surface (capability arguments, manifest fields, WASM memory, HTTP bodies) - the format invariants in `PredicateEventId::new` (non-empty, NUL-free) are a durability contract for SQL backends, NOT a trust check - a tenant-supplied id can either undercount itself into infinity by replaying a fixed id, or poison adjacent buckets if scoping is ever weakened Doc-only; no behavior change. * test(hooks): add caller-boundary replay-dedup test through wrapper hook henrypark133 HIGH blocker B1 on PR #3635: replay dedup must engage at the caller boundary — `PredicateBackedBeforeCapabilityHook::evaluate` is the production path the dispatcher invokes for installed predicate hooks. A unit test on `PredicateEvaluator::evaluate_at` alone is insufficient regression coverage (repo CLAUDE.md rule "Test through the caller, not just the helper"): the wrapper hook reads `BeforeCapabilityHookContext::caller_event_id` and threads it down to the backend, so the regression test must drive the wrapper itself. The threading work already shipped in commit e6df47d (`caller_event_id` field on the public hook context + evaluator preferring it over the synth path). This commit adds the missing end-to-end test: 1. Two `PredicateBackedBeforeCapabilityHook::evaluate` calls with the same `caller_event_id` and a `RateOrValueCap { max: 1 }` predicate — the second call must stay under cap (dedupe engages at the wrapper boundary, not be re-counted into a deny). 2. A third call with a DISTINCT `caller_event_id` crosses the cap — proving dedup is replay-scoped (same id → no-op), not blanket- suppress (any id → no-op). If the wrapper were synthesizing a fresh id per call (the bug Henry flagged before threading landed), this test would fail at step 2 with the second evaluation being denied. * docs(hooks): D5a + cross-process replay note; add caller-API tests henrypark133 should-fix S8 + S9 on PR #3635. S8 — threat-model expansion: - Add D5a as the correctness-under-attack variant of D5: an attacker flooding high-cardinality keys can LRU-evict legitimate tenants' counters and reset their rate-limit state. Distinct from the memory-only framing of D5; tied back to per-extension caps (D3/D4) and the durable-backend successor (doc 03). - Document the cross-process replay limit on the in-memory backend inside the PredicateStateBackend trait docs, not just in D5 — the process-local dedup is a property callers need at the trait surface, with a pointer to the durable backend as the cross-host story. S9 — three new tests on the in-memory backend public API: - lru_eviction_via_public_api_holds_max_history_keys_cap: drives MAX_HISTORY_KEYS + 1 distinct keys through record_invocation and asserts the map size cap holds + evictions_observed() advances. The previous coverage manually crafted buckets and called the LRU helper directly; this exercises the production path. - in_memory_invocation_retains_entry_at_exact_window_cutoff: pins the `< cutoff` trim semantics so a refactor to `<=` would fail loud. - event_id_dedup_is_isolated_across_invocation_and_value_maps: same event_id used in both record_invocation and record_value must not cross-suppress — the two maps key on disjoint types. The fourth S9 item (concurrent N-thread atomicity) and the caller- boundary replay test on the wrapper hook already landed in earlier commits (f632d22, predicate_state.rs line 840). S2 (evict_older_than stub), S3 (sync-trait docs), and S7 (consistency vs batched-writes) were also already in HEAD; this commit ships the remaining items. Quality gate: cargo fmt clean, cargo clippy -p ironclaw_hooks --all-features --tests -D warnings clean, full hooks test suite green (15 predicate_state unit tests + lib + integration). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(hooks): co-locate synth_event_id with backend; rationale comment; pin synth format henrypark133 nits N1, N2, N5 on PR #3635. N1 — Move `synth_event_id` from `evaluator.rs` to `predicate_state.rs` as `PredicateEventId::synth(...)`. The id format (64-char lowercase hex, no NUL, never empty) is part of the backend's durable contract, so co-locating with `PredicateStateBackend` keeps the format change- surface adjacent to the consumer. To avoid inverting the module dependency (`predicate_state` is a leaf below `points`), the synth helper takes raw bytes / &str rather than a `&BeforeCapabilityHookContext`. The evaluator's `resolve_event_id` fallback unpacks the context and delegates. N2 — `// safety:` comment on a non-`unsafe` block (the `write!(s, "{byte:02x}")` infallibility note) renamed to `// RATIONALE:`. By convention `// SAFETY:` pairs with `unsafe` blocks; using `// safety:` elsewhere conflates the two. N5 — Add `synth_event_id_is_64_char_lowercase_hex` to pin the synth output shape. A refactor that silently changes length or case would break the durable backend's `uuid`-shaped UNIQUE constraint without a test failure today; the new test fails loud. Quality gate: cargo fmt clean, cargo clippy --all --benches --tests --examples --all-features -D warnings clean, full hooks lib test suite green (172 passing including the new pin). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): drop expect() in hex formatting to satisfy panic CI check The "No panics in production code" CI check (scripts/check_no_panics.py) only recognizes `// safety:` suppression markers, not `RATIONALE:`. Since std::fmt::Write for String is infallible, just discard the Result with `let _ =` instead of `.expect()` — no panic call, no marker needed. Also merges in latest origin/hooks-foundation-01 (now includes the reborn-integration merge and PR #3636). * fix(hooks): narrow caller_event_id visibility to pub(crate) henrypark133 MED on PR #3635 5-19 review. The pub field let external callers bypass with_caller_event_id and assign values the validated PredicateEventId constructor would have rejected. Force every external caller through the typed setter so PredicateEventId::new is the only entry point. * fix(hooks): drop arguments_digest from synth + add in-memory backend warn Two PR #3635 5-19 review findings on the evaluator surface: - henrypark133 LOW (synth oracle): drop arguments_digest from the PredicateEventId synth hash input. The 64-char hex output was an equality oracle for argument shape; replay dedup for durable backends uses the caller-supplied caller_event_id, not the synth path, so synth only needs to be per-call unique, not content-addressed. - henrypark133 HIGH + MED (in-memory production limits): expose PredicateEvaluator::warn_in_memory_backend_active_in_production for hosts to call at startup. Multi-host replay dedup is process-local and the LRU cap is shared across tenants; operators need this surfaced in logs when the durable backend is not wired. * fix(hooks): harden predicate state backend per PR #3635 5-19 review Address five findings on crates/ironclaw_hooks/src/predicate_state.rs: - A1 (henrypark133 HIGH): restrict PredicateEventId::new_unchecked to pub(crate) so external callers cannot bypass the durable UNIQUE-constraint format invariants enforced by ::new. - A4 (henrypark133 MED): per-tenant LRU quota at MAX_HISTORY_KEYS / 4. Without it a noisy tenant could fill the global cap and evict a quiet tenant's bucket, resetting their rate-limit counter. With the quota, a tenant that overflows evicts its OWN oldest-front bucket first. New tests cover single-tenant cap and cross-tenant isolation. - A5 (henrypark133 LOW): drop arguments_digest from the synth hash input (oracle closure mirrored from the evaluator side). Add a thread-local nonce alongside the process-global counter so synth remains per-call unique without relying solely on a contended AtomicU64. New test pins the divergence invariant. - D6 (henrypark133 HIGH): O(1) NumericSum via an incrementally- maintained ValueBucket::running_sum, replacing the O(n) deque walk on every record_value call. New test covers push/trim/replay interactions. - D8 (henrypark133 MED): implement evict_older_than for the in-memory backend (was a no-op Ok(0) default). Drops entries strictly older than the cutoff and removes empty buckets; operator reaper tasks rely on this to reclaim memory from idle keys. - D7 (henrypark133 MED, partial): document the process-global synth COUNTER as a known contention hotspot and add a thread-local nonce so threads can advance without forcing cross-core invalidation in the common path. Tests: 196 passing (+4 new); workspace clippy clean. * fix(hooks): port MAX_SAMPLES_PER_KEY cap into PredicateStateBackend (D5 regression from r3) Round 3 of PR #3573 (already merged into hooks-foundation-01) added an inline per-key sample cap of 4_096 in evaluator.rs to bound memory under attacker-triggered hot capabilities with very large declared windows (threat-model finding D5). The predicate-state extraction in PR #3635 moved that bookkeeping into the PredicateStateBackend trait but missed porting the cap, so the cap would silently disappear from production once this PR rebases onto the foundation branch. This commit moves the cap into the in-memory backend impl next to MAX_HISTORY_KEYS / MAX_KEYS_PER_TENANT and enforces it in both record_invocation and record_value. For the NumericSum path, the bucket helper's pop_front already decrements running_sum, so the incremental sum invariant survives cap-driven eviction. The pre-existing inline copy in evaluator.rs becomes redundant once the trait impl owns the enforcement; the rebase resolution deletes it. Adds two regression tests: - record_invocation_caps_samples_per_key_under_attacker_pressure - record_value_evicts_oldest_keeping_running_sum_consistent * fix(hooks): port predicate_state tests to ::new() after #3912 newtype privatization --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zmanian
added a commit
that referenced
this pull request
May 23, 2026
… to #3573) (#3637) * docs(hooks): scope invocation_arguments_digest snapshot pin (successor #10) Successor PR from #3573 — addresses serrrfirat's #3 follow-up. Adds an explicit snapshot test pinning the digest for a known input so a future change to the hashing path or input-ref format is loud. * test(hooks): pin invocation_arguments_digest with snapshot Address serrrfirat's #3 follow-up from PR #3573 — partial fix promoted to full pin. Two new tests in capability_port.rs::tests: - invocation_arguments_digest_is_stable_for_known_inputs: pins the digest for a fixed (capability_id, input_ref) fixture so any future change to the hashing structure or input-ref format is loud. The captured hex is documented in the assertion message + stability contract. - invocation_arguments_digest_differs_for_different_input_refs: structural sanity check that distinct inputs produce distinct digests. Plus expanded rustdoc on the function calling out the stability contract: changing the hashing path requires updating the fixture, surfacing in the cross-crate wire-format contract section, and bumping the framework's contract version if downstream consumers exist. Tests: 156 unit tests pass (was 154; +2 new). Clippy + fmt clean. * test(hooks): pin arguments_digest at middleware boundary (serrrfirat #3637) serrrfirat MED on PR #3637: the existing snapshot test pins `invocation_arguments_digest`'s raw output, but the public hook contract is `BeforeCapabilityHookContext.arguments_digest` populated via `HookedLoopCapabilityPort::hook_context`. If caller-side wiring drifts — wrong field set, transform inserted, stale/default digest, or an alternate path bypassing the helper — the helper snapshot stays green while hook consumers observe a broken digest. Add a boundary-level pin: construct a `HookedLoopCapabilityPort` with a no-op inner port and an empty dispatcher, run the same fixed `(capability_id, input_ref)` invocation through `hook_context()`, and assert the resulting `ctx.arguments_digest` matches the same pinned hex as the helper snapshot. If they ever disagree, this assertion fails — surfacing wiring drift that the helper test alone cannot. `HookedLoopCapabilityPort::hook_context` is widened from private to `pub(crate)` to make the boundary test possible without bypassing the function. * docs(hooks): clarify arguments_digest rustdoc — input-ref identity, not arguments (serrrfirat blocker on PR #3637) The rustdoc summary on invocation_arguments_digest described the digest as covering "capability arguments" with equivalence under "identical arguments". This contradicted the new stability section in the same file, which (correctly) documents that the digest is over the (capability_id, input_ref) identity tuple — NOT over the resolved argument content the input-ref points at. Reword the summary and the equivalence claim to describe input-ref identity, matching the stability section and the actual implementation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zmanian
added a commit
that referenced
this pull request
May 23, 2026
* docs(hooks): scope event-triggered hooks (Phase 5, successor #4) Successor PR from #3573. Adds a new EventTriggered hook point that subscribes to RuntimeEvents asynchronously, outside the loop's inline tick. Observer-only by construction (no Allow/Deny/Patch); typed against a narrowed HookObservableEvent projection to keep the cross-crate boundary clean. Scope doc only; design questions about cursor/replay semantics and per-extension event-rate caps need design review before implementation. * Implement Phase 5 event-triggered hooks Cites crates/ironclaw_hooks/docs/successors/04-event-triggered-hooks.md as the scope contract. Adds the EventTriggered observer hook point, durable RuntimeEvent dispatch path, and Reborn pull-driven subscription wiring with caller-level coverage for matching, replay, scope filtering, observer-only authority, and backpressure. * Fix hook event OwnCapabilities owner lookup * fix(hooks): carry owning extension into hook milestone runtime events henrypark133 HIGH + codex P1 on PR #3640: `OwnCapabilities`-scoped event-triggered subscriptions silently never fired for `HookFailed`/`HookDecisionEmitted`/`HookDispatched` events because those `RuntimeEvent` constructors hardcoded `provider: None`. Since Installed hooks default to `OwnCapabilities`, the very events that Phase 5 was designed to observe (hook-failure / decision alerting) never reached their default-configured subscriber. A prior fix added a hook_id-based fallback in `scope_provider_for_runtime_event` that resolves the owning extension through the registry's hex index when `event.provider` is `None`. That covers the case where the failing hook is still registered at replay time, but the durable fix is to stamp the originating provider into the event at emit time so the primary `event.provider` path resolves without any fallback. Plumbed `owning_extension: Option<ExtensionId>` end-to-end: - `LoopHostMilestoneKind::{HookDispatched, HookDecisionEmitted, HookFailed}` gain the field (with `#[serde(default, skip_serializing_if = "Option::is_none")]` so pre-existing checkpoint payloads and the L3 schema-snapshot tests round-trip unchanged when no owner is set). - `RuntimeEvent::hook_{dispatched, decision_emitted, failed}` constructors accept the owner and stamp it into `provider`. - `milestone_events.rs` threads the field through the projection. - `HookDispatcher::emit_dispatched/emit_decision` pass `binding.owning_extension.clone()` directly. - `HookDispatcher::emit_failure` (no binding handy on the failure path) looks the owner up via the registry's existing `owning_extension_for_hook_hex` index. Tests: - `event_triggered_own_capabilities_matches_hook_failed_with_carried_provider`: primary-path regression — two `HookFailed` events with `provider: Some(ext_a|ext_b)` against an `OwnCapabilities` subscription scoped to ext_a; only the own-provider event fires and `event.provider == Some(ext_a)`. - Existing `event_triggered_own_capabilities_scope_resolves_hook_failed_owner_from_hook_id` remains green: passes `None` for the new arg so the fallback path is still exercised for legacy payloads. All other call sites updated to pass `None` (no owner available) or the resolved owner where applicable. * fix(hooks): validate event subscription scope against run scope (serrrfirat HIGH #1 on PR #3640) `EventTriggeredHookSubscription` accepted a caller-supplied `EventStreamKey` + `ReadScope` and used `run_context.scope.tenant_id` as the hook context's tenant — with no validation that the two agreed. A caller wiring tenant A's host with tenant B's stream would cause hooks to observe B's events while the hook context claimed tenant A. Cross-tenant trust-boundary break. Add `EventTriggeredHookSubscription::validate_against_run_scope` and call it from `build_text_only_host_with_capabilities` before spawning. Validation: - Stream `(tenant_id, user_id, agent_id)` must equal `(run_context.scope.tenant_id, thread_scope.owner_user_id, run_context.scope.agent_id)`. - Thread without `owner_user_id` cannot bind any subscription — the user dimension is required to verify stream identity. - Every `Some(want)` in `ReadScope` must equal the corresponding run/thread scope value (project/mission/thread). `None` is permissive (run scope owns the dimension authoritatively). Failures surface as `RebornLoopDriverHostError::ScopeMismatch` with a specific reason naming the offending dimension. Tests: - `event_triggered_subscription_with_foreign_tenant_stream_fails_host_build` - `event_triggered_subscription_with_foreign_user_stream_fails_host_build` The integration fixture's `ThreadScope` now sets `owner_user_id: Some(...)` so it passes validation; previously it was `None`, which the new check (correctly) refuses. Existing tests continue to pass. * fix(hooks): surface event subscription replay-gap as a milestone (serrrfirat MED on PR #3640) When the durable event log returned `EventError::ReplayGap`, the event-triggered subscription's background task previously logged a `tracing::warn!` and broke out of the poll loop — silently killing all future hook event delivery for the run with no operator-visible signal. A scoped audit hook that mattered to compliance would just stop, and nobody downstream would know. Surface the termination through the host's milestone sink: - New `LoopDriverNoteKind::EventSubscriptionTerminated` variant. - The subscription's `spawn`/`run` now takes the host's `Arc<dyn LoopHostMilestoneSink>` and the active `LoopRunContext`. On `ReplayGap`, it constructs a `DriverNote` milestone with that kind plus a `LoopSafeSummary` describing the gap, publishes it through the same sink that carries every other host milestone, and *then* breaks (fail-closed: the at-most-once contract is already broken; resuming from `earliest` would silently lose the gap). - Log level bumped from `warn` to `error` to match the severity. - A best-effort send: failures to publish the milestone are logged but do not stall the subscription teardown. Tests: - `event_triggered_replay_gap_emits_subscription_terminated_milestone`: appends 3 events, `truncate_before_or_at` to cursor 2 to force a replay gap, starts the subscription from cursor origin (now stale), and asserts a `DriverNote { kind: EventSubscriptionTerminated, .. }` shows up on the host's milestone sink within a 2s deadline. Self-emit reentrancy (serrrfirat MED #3 on the same PR) is intentionally not addressed here — that fix needs a design call (task-local re-entry flag vs. removing RuntimeEvent emit capability from event-hook execution contexts) and is a follow-up. * fix(hooks): suppress event-triggered self-observation (serrrfirat MED #3 on PR #3640) A hook that subscribes to one of the hook-lifecycle event kinds (`HookDispatched`/`HookDecisionEmitted`/`HookFailed`) with a scope that matches its own provider would otherwise be dispatched for events describing its OWN executions. The dispatcher emits those events itself when running the hook, so a hook subscribing to `HookFailed` with `OwnCapabilities` against its own extension would fail → emit HookFailed → re-dispatch → fail → emit → … storm. `dispatch_event_triggered_at` now skips events whose `event.hook_id` equals the binding's own hook id when the event kind is a hook- lifecycle kind (`is_hook_lifecycle_kind`). The check is intentionally narrow: - It only fires for hook-lifecycle events. Subscriptions to other event kinds are unaffected. - It only suppresses literal self-observation; events about other hooks (even hooks from the same extension) still dispatch. This does NOT cover the broader case of a hook that captures an `Arc<DurableEventLog>` and mints arbitrary `RuntimeEvent`s from inside its `observe()`. That requires architectural restriction on what hook impls can capture — tracked separately as a follow-up. Tests: - `event_triggered_self_lifecycle_event_does_not_redispatch`: appends two `HookFailed` events with the same provider — one targeting the subscriber's own hook id, one targeting a different hook. Asserts only the OTHER hook's failure fires (proves the filter is narrow, not blanket). * fix(hooks): address henrypark133 should-fix #1, #2, #3, #6 on PR #3640 Four items from the 5-15 review (#4 DoS budget and #5 narrowed projection deferred — see below): **#1 (should-fix) Invariant: EventTriggered ↔ event_kind_filter** `HookRegistry::insert` now enforces the biconditional at install time: an `EventTriggered` binding must declare an `event_kind_filter` (otherwise the dispatcher's kind match would silently never fire — a no-op binding), and conversely only `EventTriggered` bindings may declare a filter (other points are kind-agnostic and would ignore the field). Misconfigured bindings fail loud at install. **#2 (should-fix) Remove `Clone` derive on EventTriggeredHookSubscription** `Clone` on a spawn-semantics type was a footgun: external callers cloning + spawning twice would create two consumers reading from the same `start_cursor`, each dispatching every hook. Replace with an explicit `clone_for_independent_spawn(&self)` method named verbosely so the property is visible at the seam. Internal use updated in the factory's host-build path; external callers can no longer accidentally construct a dual-consumer pattern. **#3 (should-fix) catch_unwind around the background `run()` task** The subscription's tokio task body now runs inside `AssertUnwindSafe(...).catch_unwind()`; a panic in `run()` emits the same `EventSubscriptionTerminated` `DriverNote` milestone the `ReplayGap` path already emits, instead of silently terminating with no operator-visible signal. **#6 (should-fix) Replay semantics in rustdoc on public API** Added a "Replay semantics" section to `EventTriggeredHookSubscription` rustdoc: at-least-once, caller-owned cursor persistence, the restart-from-start_cursor replay pattern. Previously only in the design doc; now load-bearing API contract is visible at the type. **#4 (deferred) Per-hook DoS budget for Installed tier** Henry's recommendation was to gate `Installed`-tier event-triggered hooks entirely until the budget design lands, allowing only Builtin/Trusted. That breaks 11 existing tests + the primary use case. Instead: documented the existing first-line throttle (`batch_limit` × `poll_interval`) as the current bound on indirect- recursion fanout, and tracked the full per-hook rate cap with poisoning + milestone-on-overrun as a follow-up. The self-trigger guard (committed earlier in this PR) catches the most common direct pattern; the throttle here bounds the indirect pattern until the proper budget lands. **#5 (deferred) Narrowed `HookObservableEvent` projection** Would prevent full `RuntimeEvent` surface from reaching Installed- tier hooks. Project-wide impact (events crate types, projection glue). Tracked as a follow-up; the existing sanitized-event projection bounds the surface to closed-vocab labels. All 156 hooks lib + 30 reborn integration tests pass. * chore(hooks): address nits from PR #3640 review Bundle three nit-tier review items into a single commit: **#9 Replace author-internal tags with NOTE(#3640)** The Phase-5 PR (#3640) had several `serrrfirat HIGH/MED #N on PR #3640` comment tags in this PR's diff. These are review-internal scaffolding, not load-bearing for future readers. Replaced with `NOTE(#3640)` in: - crates/ironclaw_hooks/src/dispatch.rs (self-observation guard) - crates/ironclaw_reborn/src/loop_driver_host.rs (scope validation, replay-gap milestone, subscription binding) - crates/ironclaw_reborn/tests/hooks_integration.rs (three regression tests covering scope validation, self-observation suppression, and replay-gap surfacing) - crates/ironclaw_turns/src/run_profile/host.rs (`EventSubscriptionTerminated` doc) **#10 Replace 10ms spin-poll with tokio::sync::Notify** `wait_for_seen_events` polled the shared `Mutex<Vec<SeenRuntimeEvent>>` every 10 ms until the expected count was reached. Replaced with a `SeenLog` newtype that pairs the events vec with a `Notify`; the hook's `observe()` calls `seen.push(...)` which signals `Notify::notify_one`, and `wait_for_seen_events` parks on `notified().await` under a `tokio::time::timeout`. `notify_one` is a permit-store, so an event landing between snapshot and wait still wakes the waiter immediately. Test latency drops from ~10 ms median to sub-ms and is no longer rate-limited by the polling cadence. All 30 hooks_integration tests still pass. **#11 Remove unused Clone derive on EventTriggeredHookContext** No call site clones the context — it's passed by reference. Dropped the derive to make the borrow contract clearer. * docs(hooks): reconcile event-triggered hooks design doc with Phase 5 reality Address gemini-code-assist review on `04-event-triggered-hooks.md`: - L50 (Likely surface): annotated the sketch's full `RuntimeEvent` use with a pointer to the narrowed-projection follow-up so the snippet no longer reads as a recommendation contradicting L119–121. - L55 (sink methods): replaced `note_fact` / `emit_audit` (which never shipped on `ObserverSink`) with the actual `note(category, summary)` primitive and cross-referenced Reborn's `EventTriggeredObserverSink`. - L95 (cursor / replay): "lost events during downtime acceptable" contradicted the at-least-once replay semantics described in the Phase 5 implementation notes. Rewrote the bullet to say replay is at-least-once from the persisted cursor and to spell out the operator obligation around cursor persistence before shutdown. - L100/115 (forbids events dep): the original doc claimed `ironclaw_hooks` forbids an `ironclaw_events` dep, but the Risk section noted the dep is already established via PR #3573. Updated both passages to reflect that the dep direction is set; Phase 5 adds the *consumer* side. The narrowed `HookObservableEvent` projection is now framed as a follow-up tracked in #3690. * refactor(hooks): unify event-triggered sink with ObserverSink Address PR #3640 review findings A3, C4, F14, and cluster G: - F14: drop duplicate `EventTriggeredObserverSink` trait and reuse `ObserverSink` directly in the `EventTriggeredHook` trait. The two surfaces were signature-identical; keeping them separate let them drift, and a future gate/mutator method added to one would not surface as a compile error on the other. - A3: add `is_replay: bool` to `EventTriggeredHookContext` and a dedicated `dispatch_event_triggered_replay_at` entry point. The subscription contract is at-least-once, so side-effecting hooks need to dedupe by `event.event_id` on restart-driven replay. - C4: index event-triggered bindings by `RuntimeEventKind` at install time so dispatch is O(matches) instead of scanning every event-triggered binding for every event. - Cluster G: doc/04-event-triggered-hooks.md updated to reflect the unified sink, the explicit at-least-once semantics + `is_replay` signal, the actual `note(category, summary)` primitive (not the speculative `note_fact` / `emit_audit`), the corrected `ironclaw_events` dep status, and the issue #3690 reference for the narrowed `HookObservableEvent` projection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * perf(hooks): adaptive backoff for event-triggered subscription Address PR #3640 review findings C5, A1, A2: - C5: empty-poll backoff for `EventTriggeredHookSubscription`. The previous loop hammered the durable log at a fixed 50ms cadence under sustained idle, even when no events had arrived for minutes. The subscription now tracks consecutive empty polls and sleeps for `min(poll_interval << streak, max_poll_interval)` before the next poll, defaulting to a 1s cap; a non-empty batch resets the streak so producer bursts restore low-latency dispatch immediately. Exposed via `with_max_poll_interval` so callers can tune. - A1 / A2: explicit issue references for the deferred narrowed `HookObservableEvent` projection (#3690) and the per-hook DoS dispatch budget (#3689). The current self-trigger guard catches direct-recursion storms; the backoff bounds indirect ones until the proper budget design lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(hooks): cover event-triggered dispatch edge cases Address PR #3640 review findings D8, D9, D10, D11, D12, and B: - D8: dispatching an event-triggered binding that has no installed hook impl must poison the slot and surface a Malformed failure rather than silently no-op. A follow-up dispatch on the same kind must skip the poisoned slot. - D9: registry validation rejects non-event-point bindings that carry an `event_kind_filter`, mirroring the existing reverse-direction check. - D10: the existing hook-meta serde round-trip tests always passed `None` for `owning_extension` and never asserted `event.provider`. Add `hook_meta_events_round_trip_owning_extension_as_provider` to pin the projection that scope filtering depends on. - D11: `scope_provider_for_runtime_event` falls back to `None` when the registry mutex is poisoned. Force a poison on a spawned thread and assert the resolver remains fail-closed. - D12: `run_event_triggered_hook` catches panics from the hook impl via `AssertUnwindSafe::catch_unwind`. Drive it with a deliberately panicking impl and assert `FailureCategory::Panic`. - Cluster B: when a hook-meta event has `provider: None`, the dispatcher recovers the owning extension from the registry's hex-keyed index so `OwnCapabilities` watchers still fire. Add a full end-to-end test exercising that path through `dispatch_event_triggered_at`. Also pin C4 indexing: a registry-level test that `active_for_event_kind` returns only bindings whose declared filter matches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): port event-triggered tests after foundation rebases (#3911/#3912/#3913) - Add event_kind_filter: None to HookBinding test constructions (foundation added new field) - Replace tuple-struct construction of ExtensionId/HookLocalId with ::new() per #3912 newtype privatization - Lowercase RuntimeEventKind debug repr for HookLocalId validation (lowercase-only identifiers) - Replace pub-use re-exports with module-path imports per foundation cleanup - Rename fixture.user_id to fixture.actor_id per #3633 final naming * fix(hooks): adapt event-triggered to WASM hook runtime (#3920) - Add event_kind_filter: None to WASM HookBinding constructions in dispatch.rs - Extend HookManifestKind match arms in registrar.rs and wasm/runtime.rs to handle EventTriggered (rejected: WASM-bodied event-triggered hooks are not yet supported) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 23, 2026
zmanian
added a commit
that referenced
this pull request
Jun 6, 2026
…ED flag (#3934) (#3938) * feat(hooks): extension-declared hook section on ExtensionManifestV2 (#3934) Add a `[[hooks]]` declaration surface to the production v2 extension manifest (`ironclaw_extensions::ExtensionManifestV2` and its projected `ExtensionManifest`). Each entry is carried as a structurally-typed `HookSectionEntryV2` DTO — a `local_id` plus the entry's body re-serialized to canonical TOML — so `ironclaw_extensions` (substrate) never imports the `ironclaw_hooks` predicate vocabulary. The composition layer, which depends on both crates, is the single seam that projects these payloads into typed `ironclaw_hooks::HookManifestEntry` values (a later commit). Parse-time structural bounds: `MAX_MANIFEST_HOOKS` (32, matching the downstream per-extension registration cap) and `MAX_HOOK_ENTRY_BYTES` (8 KiB per entry). Entries must be tables carrying a non-empty `id`; ids must be unique within the manifest. `#[serde(default)]` keeps every existing manifest valid (empty `hooks` vec). The DTO holds canonical TOML as a `String` rather than a `toml::Value` so the enclosing `ExtensionManifestV2` keeps its `Eq` derive (`toml::Value` is not `Eq`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(hooks): composition-layer activation module (loader, first-party hook, flag) (#3934) Add `ironclaw_reborn_composition::hooks` — the single seam that activates the hook framework in production. Implements four numbered pieces of #3934: - Feature flag (item 7): `HooksActivationConfig`, default OFF, resolved from `HOOKS_ENABLED` (only `1`/`true`/`yes`/`on` enable; unset/anything else = OFF). Flag OFF ⇒ `build_hook_dispatcher_builder_factory` returns `None` and the runtime composes no dispatcher — exact pre-hooks behavior. Hard rollout-safety contract. - Manifest → registry loader (item 2): `install_extension_hooks` projects each `ExtensionManifestV2` `HookSectionEntryV2` (canonical TOML) into a typed `HookManifestEntry` and installs it via `HookRegistrar::install` at the `Installed` trust tier. This is the clean-boundary projection: the hook vocabulary lives only here, never in `ironclaw_extensions`. Trust attenuation is enforced by construction (registrar only calls `install_installed_*`). Fail-closed: any projection/install error fails the build loudly. - First-party builtin hooks (item 3): a single illustrative no-op observer (`NoOpObserverHook`), installed regardless of extensions. Ships dark (zero driver-visible effect even with the flag ON). Production catalog is TBD by design — this PR does not invent a first-party hook. - Dispatcher composition (item 5): builds a per-tenant `PredicateEvaluator` over the in-memory state backend (swappable via the new public `PredicateEvaluator::with_state_backend` for durable #3933), validates the full install set once fail-closed, and returns a per-run builder-factory closure. Per-run construction (fresh registry/dispatcher per host build) + per-tenant evaluator give full isolation; the host factory attaches the run-scoped milestone sink internally. Per-tenant scoping is by construction: `build_reborn_runtime` runs once per identity, so everything here is tenant-local — no global registry. The router-backed gate-ref factory (PauseApproval/PauseAuth) and the security-audit sink (#3922, not yet on this branch) are deferred follow-ups; their absence is fail-closed (PauseApproval surfaces as Denied) and noted for the PR body. Not yet wired into the runtime — next commit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(hooks): wire dispatcher builder factory into build_default_planned_runtime (#3934) Item 6 of #3934. Add an optional `hook_dispatcher_builder_factory` to `DefaultPlannedRuntimeParts` and, in `build_default_planned_runtime`, call `.with_hook_dispatcher_builder_factory(...)` on the production `RebornLoopDriverHostFactory` when it is present. `None` (the default) means no dispatcher is composed — behavior identical to the pre-hooks runtime (rollout-safety contract). The composition layer (`build_reborn_runtime`) resolves the flag via `HooksActivationConfig::from_env()` and builds the factory against this tenant's extension registry (per-tenant by construction — the function runs once per identity). Fail-closed: a malformed manifest hook fails the build here rather than composing a broken dispatcher. A per-run builder factory (not a captured dispatcher instance) is used so the host attaches a run-scoped milestone sink internally per build — per-run telemetry attribution, the #3573 capture-and-stick lesson. All `DefaultPlannedRuntimeParts` construction sites (8 test sites across ironclaw_reborn, ironclaw_product_workflow, ironclaw_reborn_composition) updated with the new field defaulting to `None`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): e2e activation tests through build_default_planned_runtime (#3934) Item 8 of #3934. Add four end-to-end tests in crates/ironclaw_reborn/tests/loop_driver_host.rs that drive the *production* composition function `build_default_planned_runtime` with a per-run hook dispatcher builder factory shaped exactly like the composition layer's output (first-party builtin no-op observer + extension-declared `Installed`-tier hooks projected from a manifest entry through `HookRegistrar::install`), then build a host via the composed `host_factory` and invoke a capability: - flag OFF (no factory): allowed capability completes unaffected and reaches the inner host runtime port — the pre-hooks behavior / rollout-safety contract. - flag ON, first-party-only no-op observer: outcome unchanged, inner port reached — the builtin ships dark. - flag ON, extension-declared deny hook: capability denied through the composed runtime and the inner port is never reached (installed at the Installed tier via the registrar; OwnCapabilities scope keyed to the capability provider). - per-tenant isolation: tenant A's deny hook fires; tenant B (separate build_default_planned_runtime composition, no hooks) completes the same capability — proving no cross-tenant leakage. Security-audit-on-deny assertion is intentionally deferred: #3922's SecurityAuditSink is not yet on reborn-integration. It lands with the audit-sink wiring follow-up. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): thread hook_dispatcher_builder_factory through shared reborn harness (#3934) The root-crate `tests/support/reborn/harness.rs` constructs `DefaultPlannedRuntimeParts` directly; add the new `hook_dispatcher_builder_factory: None` field so the parity-test harness compiles. Default `None` keeps the harness on the no-hooks path (unchanged behavior). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks): proper error handling / safety annotations for activation production paths (#3938 CI) The per-run dispatcher factory closure used `.expect()` on the first-party and extension hook installs, tripping the no-panics CI gate. These installs are pure replays of the install set already validated fail-closed (`?`) against a scratch builder at composition time, so they are genuine invariants. The factory type returns a non-Result `HookDispatcherBuilder` and is invoked deep in the run loop, so the documented `// safety:` suppression is the correct fix here. Hoisted the expect messages into `let` bindings so the `.expect(msg)` call fits on one line, keeping the scanner-required `// safety:` comment on the same line as the call after rustfmt. The malformed-manifest path (TOML projection) already uses real error propagation via map_err/`?` and is unaffected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): direct composition-loader coverage + activation-scope docs Address Codex non-blocking follow-ups on #3938. Add three direct tests for the composition-layer hook loader (`install_extension_hooks` via `build_hook_dispatcher_builder_factory`), driving a real `ExtensionRegistry` with `[[hooks]]` declarations rather than mimicking the loader: - valid `own_capabilities` predicate hook installs at the Installed trust tier; the dispatcher carries the derived binding at BeforeCapability alongside the first-party no-op observer - malformed typed hook body (unknown `mode`) fails CLOSED with `RebornBuildError::InvalidConfig`, never a panic (the load-bearing degradation contract for untrusted external manifests) - a hook claiming `scope = same_tenant` without a verified grant is rejected by trust attenuation (fail-closed) No loader bug surfaced: `HookRegistrar::install` already returns `Result` on every malformed/over-scoped path and the loader maps it to `InvalidConfig` via `?`. Document activation scope at both the loader rustdoc and the `build_reborn_runtime` call site: production currently passes only `builtin_extension_registry()`, so third-party installed-extension hooks are not yet surfaced into the runtime path — only first-party-builtin and builtin-package-declared hooks activate today. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hooks): thread HooksActivationConfig through input; empty production catalog Two maintainability cleanups on #3938 (firat review): Item 1 — config hygiene: stop reading HOOKS_ENABLED via std::env::var deep inside build_reborn_runtime. Add a typed `hooks: HooksActivationConfig` field to RebornRuntimeInput (default OFF) plus a `with_hooks_config` builder. The composition root now consumes the typed config; the env var is resolved ONCE at the edge (the reborn CLI's build_runtime_input) via HooksActivationConfig::from_env and threaded down. Testable without env mutation; matches the project's env → typed config → composition pattern. Item 2 — empty production first-party catalog: stop shipping NoOpObserverHook as a first-party builtin. install_first_party_hooks is now a no-op (empty catalog); the production type/install/export for a hook that does nothing is gone (removed from lib.rs exports). The activation machinery is still tested end-to-end through the real composition path via a new `build_hook_dispatcher_builder_factory_with` seam that takes a first-party installer; tests pass a `#[cfg(test)]` NoOpObserverHook through it. Pinned the empty-catalog-is-valid contract: flag ON + empty first-party set + no extension hooks composes a valid zero-binding dispatcher (not a panic/error). Reconciled the sibling loader tests/docs (f6c79c0): the in-module tests now drive the test-only seam; the reborn e2e tests already used a test-local no-op and are untouched. Updated activation-scope docs (loader rustdoc + build_reborn_runtime call site) to reflect the now-single live source (builtin-package-declared hooks). Deferred (not touched): switching to the canonical extension registry for third-party installed-extension hooks (#3934 follow-on). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hooks): canonical registry + infallible plan + tenant-scoped counter docs/tests (#3938) Addresses serrrfirat's thermo-nuclear re-review on 1e618d0. #1 (runtime.rs:839, canonical registry): make the extension registry a shared composition artifact. `build_local_dev` builds one `Arc<ExtensionRegistry>`, hands it to `HostRuntimeServices::new` AND stores it in `RebornLocalRuntimeServices.extension_registry`. Hook activation in `build_reborn_runtime` now consumes that same `Arc` instead of rebuilding a builtin-only sidecar, so capability dispatch and hook activation cannot drift. Third-party activation stays a follow-up, but it now follows the canonical registry rather than a separate path. #3 (hooks.rs factory machinery): replace the parse/validate/replay duplication + two prose-justified `.expect()` calls with a typed `HookInstallPlan`. TOML is projected once into typed entries, the full install set is validated once against a fresh builder (fail-closed via `?`), and `HookInstallPlan::rebuild` mints a fresh builder per run. The per-run path is infallible by construction: a plan only exists for an install set that already composed cleanly, so a deterministic replay from the identical fresh-empty start cannot fail. One extension-install code path (`project_extension_install_sets` + `install_extension_sets`) is shared by validation and rebuild. #4 (hooks.rs:295, predicate counter scoping): the evaluator/backend is intentionally tenant-scoped and shared across runs (rate/value caps keyed `(hook, tenant, capability)` with no run_id; a run-scoped limit would reset every run and enforce nothing). Document the split explicitly — per-run-fresh dispatcher, tenant-scoped predicate counters — in the module docs and fix the misleading "per-run isolation of hook state" wording in `ironclaw_reborn` (loop_driver_host.rs / runtime.rs). Add `predicate_counter_state_is_tenant_scoped_across_rebuilds`, which drives a real rate-cap predicate through two dispatchers from one factory and proves the second run sees the first run's recorded count. Rename `factory_mints_independent_dispatchers_per_call` -> `rebuild_mints_independent_dispatchers_per_call` and scope it to proving dispatcher freshness only. #6 (loop_driver_host tests): clarify that the hand-built builder factories cover host PLUMBING, not composition activation. Add `build_reborn_runtime_activates_hooks_through_real_composition_path`, which drives the real `build_reborn_runtime` with `HooksActivationConfig` threaded through `RebornRuntimeInput` (env-free) and the canonical registry, proving the production activation wiring composes. #2 (env boundary) and #5 (empty production catalog) were already fixed in 1e618d0; docs touched here for consistency. Known follow-up (not one of the six items, not introduced here): with the flag ON the standalone local-dev runtime does not yet reach `Completed` for a capability turn even with a zero-binding dispatcher — the composition root wires the dispatcher but not the companion hooked-prompt dependencies. The new runtime test asserts `is_terminal()` + the capability path and documents the gap. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): cover hook-entry rejection branches; document InvocationCount cap semantics (#3938) Address henrypark133 review (review 4367870023): - Add extension-manifest tests for the three previously-uncovered hook-entry validation branches: non-table `[[hooks]]` element, whitespace-only `id`, and oversized entry (HookEntryTooLarge). - Document the InvocationCount inclusive-allow / deny-on-overflow semantics inline at the comparison site; behavior unchanged and still pinned by the cap test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(reborn-cli): assert hooks config threaded in caller test (#3938) Addresses the review finding that `build_runtime_input_maps_configured_cli_identity` exercised `build_runtime_input` but never asserted the `hooks` field, so a regression silently dropping `with_hooks_config(HooksActivationConfig::from_env())` or flipping the default-OFF rollout-safety contract would pass. Per `.claude/rules/testing.md` ("Test Through the Caller"): add two assertions to the existing caller-level test: - threading: `runtime_input.hooks == HooksActivationConfig::from_env()`, proving the env-resolved config is actually threaded through and not dropped. Verified via TDD that dropping the wiring fails this assertion (run under HOOKS_ENABLED=1). - default-OFF: when `HOOKS_ENABLED` is unset, `!runtime_input.hooks.is_enabled()`, guarded to skip if the CI environment exports the flag so it only pins the contract it claims to. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(hooks): correct in-memory backend warning text; delegate test ctor (#3938) Address serrrfirat review (2026-06-03): - Low: the in-memory backend warning claimed the LRU cap is shared across tenants, but the Reborn composition constructs a fresh InMemoryPredicateStateBackend per tenant. Rewrite the warn! text and doc comment so the real limitation (process-local replay dedup for multi-host deployments) is accurate, and note the backend is per-tenant in this composition. - Nit: PredicateEvaluator::with_backend (test-only) and with_state_backend had identical bodies; delegate with_backend to with_state_backend so they stay in lockstep. The Medium finding (hooks_config assertion in build_runtime_input caller test) was already addressed in 218a1de. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
zmanian
added a commit
that referenced
this pull request
Jun 6, 2026
…ection (HOOKS_THIRD_PARTY_ENABLED, default OFF) (#3951) * feat(hooks): extension-declared hook section on ExtensionManifestV2 (#3934) Add a `[[hooks]]` declaration surface to the production v2 extension manifest (`ironclaw_extensions::ExtensionManifestV2` and its projected `ExtensionManifest`). Each entry is carried as a structurally-typed `HookSectionEntryV2` DTO — a `local_id` plus the entry's body re-serialized to canonical TOML — so `ironclaw_extensions` (substrate) never imports the `ironclaw_hooks` predicate vocabulary. The composition layer, which depends on both crates, is the single seam that projects these payloads into typed `ironclaw_hooks::HookManifestEntry` values (a later commit). Parse-time structural bounds: `MAX_MANIFEST_HOOKS` (32, matching the downstream per-extension registration cap) and `MAX_HOOK_ENTRY_BYTES` (8 KiB per entry). Entries must be tables carrying a non-empty `id`; ids must be unique within the manifest. `#[serde(default)]` keeps every existing manifest valid (empty `hooks` vec). The DTO holds canonical TOML as a `String` rather than a `toml::Value` so the enclosing `ExtensionManifestV2` keeps its `Eq` derive (`toml::Value` is not `Eq`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(hooks): composition-layer activation module (loader, first-party hook, flag) (#3934) Add `ironclaw_reborn_composition::hooks` — the single seam that activates the hook framework in production. Implements four numbered pieces of #3934: - Feature flag (item 7): `HooksActivationConfig`, default OFF, resolved from `HOOKS_ENABLED` (only `1`/`true`/`yes`/`on` enable; unset/anything else = OFF). Flag OFF ⇒ `build_hook_dispatcher_builder_factory` returns `None` and the runtime composes no dispatcher — exact pre-hooks behavior. Hard rollout-safety contract. - Manifest → registry loader (item 2): `install_extension_hooks` projects each `ExtensionManifestV2` `HookSectionEntryV2` (canonical TOML) into a typed `HookManifestEntry` and installs it via `HookRegistrar::install` at the `Installed` trust tier. This is the clean-boundary projection: the hook vocabulary lives only here, never in `ironclaw_extensions`. Trust attenuation is enforced by construction (registrar only calls `install_installed_*`). Fail-closed: any projection/install error fails the build loudly. - First-party builtin hooks (item 3): a single illustrative no-op observer (`NoOpObserverHook`), installed regardless of extensions. Ships dark (zero driver-visible effect even with the flag ON). Production catalog is TBD by design — this PR does not invent a first-party hook. - Dispatcher composition (item 5): builds a per-tenant `PredicateEvaluator` over the in-memory state backend (swappable via the new public `PredicateEvaluator::with_state_backend` for durable #3933), validates the full install set once fail-closed, and returns a per-run builder-factory closure. Per-run construction (fresh registry/dispatcher per host build) + per-tenant evaluator give full isolation; the host factory attaches the run-scoped milestone sink internally. Per-tenant scoping is by construction: `build_reborn_runtime` runs once per identity, so everything here is tenant-local — no global registry. The router-backed gate-ref factory (PauseApproval/PauseAuth) and the security-audit sink (#3922, not yet on this branch) are deferred follow-ups; their absence is fail-closed (PauseApproval surfaces as Denied) and noted for the PR body. Not yet wired into the runtime — next commit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(hooks): wire dispatcher builder factory into build_default_planned_runtime (#3934) Item 6 of #3934. Add an optional `hook_dispatcher_builder_factory` to `DefaultPlannedRuntimeParts` and, in `build_default_planned_runtime`, call `.with_hook_dispatcher_builder_factory(...)` on the production `RebornLoopDriverHostFactory` when it is present. `None` (the default) means no dispatcher is composed — behavior identical to the pre-hooks runtime (rollout-safety contract). The composition layer (`build_reborn_runtime`) resolves the flag via `HooksActivationConfig::from_env()` and builds the factory against this tenant's extension registry (per-tenant by construction — the function runs once per identity). Fail-closed: a malformed manifest hook fails the build here rather than composing a broken dispatcher. A per-run builder factory (not a captured dispatcher instance) is used so the host attaches a run-scoped milestone sink internally per build — per-run telemetry attribution, the #3573 capture-and-stick lesson. All `DefaultPlannedRuntimeParts` construction sites (8 test sites across ironclaw_reborn, ironclaw_product_workflow, ironclaw_reborn_composition) updated with the new field defaulting to `None`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): e2e activation tests through build_default_planned_runtime (#3934) Item 8 of #3934. Add four end-to-end tests in crates/ironclaw_reborn/tests/loop_driver_host.rs that drive the *production* composition function `build_default_planned_runtime` with a per-run hook dispatcher builder factory shaped exactly like the composition layer's output (first-party builtin no-op observer + extension-declared `Installed`-tier hooks projected from a manifest entry through `HookRegistrar::install`), then build a host via the composed `host_factory` and invoke a capability: - flag OFF (no factory): allowed capability completes unaffected and reaches the inner host runtime port — the pre-hooks behavior / rollout-safety contract. - flag ON, first-party-only no-op observer: outcome unchanged, inner port reached — the builtin ships dark. - flag ON, extension-declared deny hook: capability denied through the composed runtime and the inner port is never reached (installed at the Installed tier via the registrar; OwnCapabilities scope keyed to the capability provider). - per-tenant isolation: tenant A's deny hook fires; tenant B (separate build_default_planned_runtime composition, no hooks) completes the same capability — proving no cross-tenant leakage. Security-audit-on-deny assertion is intentionally deferred: #3922's SecurityAuditSink is not yet on reborn-integration. It lands with the audit-sink wiring follow-up. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): thread hook_dispatcher_builder_factory through shared reborn harness (#3934) The root-crate `tests/support/reborn/harness.rs` constructs `DefaultPlannedRuntimeParts` directly; add the new `hook_dispatcher_builder_factory: None` field so the parity-test harness compiles. Default `None` keeps the harness on the no-hooks path (unchanged behavior). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks): proper error handling / safety annotations for activation production paths (#3938 CI) The per-run dispatcher factory closure used `.expect()` on the first-party and extension hook installs, tripping the no-panics CI gate. These installs are pure replays of the install set already validated fail-closed (`?`) against a scratch builder at composition time, so they are genuine invariants. The factory type returns a non-Result `HookDispatcherBuilder` and is invoked deep in the run loop, so the documented `// safety:` suppression is the correct fix here. Hoisted the expect messages into `let` bindings so the `.expect(msg)` call fits on one line, keeping the scanner-required `// safety:` comment on the same line as the call after rustfmt. The malformed-manifest path (TOML projection) already uses real error propagation via map_err/`?` and is unaffected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): direct composition-loader coverage + activation-scope docs Address Codex non-blocking follow-ups on #3938. Add three direct tests for the composition-layer hook loader (`install_extension_hooks` via `build_hook_dispatcher_builder_factory`), driving a real `ExtensionRegistry` with `[[hooks]]` declarations rather than mimicking the loader: - valid `own_capabilities` predicate hook installs at the Installed trust tier; the dispatcher carries the derived binding at BeforeCapability alongside the first-party no-op observer - malformed typed hook body (unknown `mode`) fails CLOSED with `RebornBuildError::InvalidConfig`, never a panic (the load-bearing degradation contract for untrusted external manifests) - a hook claiming `scope = same_tenant` without a verified grant is rejected by trust attenuation (fail-closed) No loader bug surfaced: `HookRegistrar::install` already returns `Result` on every malformed/over-scoped path and the loader maps it to `InvalidConfig` via `?`. Document activation scope at both the loader rustdoc and the `build_reborn_runtime` call site: production currently passes only `builtin_extension_registry()`, so third-party installed-extension hooks are not yet surfaced into the runtime path — only first-party-builtin and builtin-package-declared hooks activate today. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hooks): thread HooksActivationConfig through input; empty production catalog Two maintainability cleanups on #3938 (firat review): Item 1 — config hygiene: stop reading HOOKS_ENABLED via std::env::var deep inside build_reborn_runtime. Add a typed `hooks: HooksActivationConfig` field to RebornRuntimeInput (default OFF) plus a `with_hooks_config` builder. The composition root now consumes the typed config; the env var is resolved ONCE at the edge (the reborn CLI's build_runtime_input) via HooksActivationConfig::from_env and threaded down. Testable without env mutation; matches the project's env → typed config → composition pattern. Item 2 — empty production first-party catalog: stop shipping NoOpObserverHook as a first-party builtin. install_first_party_hooks is now a no-op (empty catalog); the production type/install/export for a hook that does nothing is gone (removed from lib.rs exports). The activation machinery is still tested end-to-end through the real composition path via a new `build_hook_dispatcher_builder_factory_with` seam that takes a first-party installer; tests pass a `#[cfg(test)]` NoOpObserverHook through it. Pinned the empty-catalog-is-valid contract: flag ON + empty first-party set + no extension hooks composes a valid zero-binding dispatcher (not a panic/error). Reconciled the sibling loader tests/docs (f6c79c0): the in-module tests now drive the test-only seam; the reborn e2e tests already used a test-local no-op and are untouched. Updated activation-scope docs (loader rustdoc + build_reborn_runtime call site) to reflect the now-single live source (builtin-package-declared hooks). Deferred (not touched): switching to the canonical extension registry for third-party installed-extension hooks (#3934 follow-on). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hooks): canonical registry + infallible plan + tenant-scoped counter docs/tests (#3938) Addresses serrrfirat's thermo-nuclear re-review on 1e618d0. #1 (runtime.rs:839, canonical registry): make the extension registry a shared composition artifact. `build_local_dev` builds one `Arc<ExtensionRegistry>`, hands it to `HostRuntimeServices::new` AND stores it in `RebornLocalRuntimeServices.extension_registry`. Hook activation in `build_reborn_runtime` now consumes that same `Arc` instead of rebuilding a builtin-only sidecar, so capability dispatch and hook activation cannot drift. Third-party activation stays a follow-up, but it now follows the canonical registry rather than a separate path. #3 (hooks.rs factory machinery): replace the parse/validate/replay duplication + two prose-justified `.expect()` calls with a typed `HookInstallPlan`. TOML is projected once into typed entries, the full install set is validated once against a fresh builder (fail-closed via `?`), and `HookInstallPlan::rebuild` mints a fresh builder per run. The per-run path is infallible by construction: a plan only exists for an install set that already composed cleanly, so a deterministic replay from the identical fresh-empty start cannot fail. One extension-install code path (`project_extension_install_sets` + `install_extension_sets`) is shared by validation and rebuild. #4 (hooks.rs:295, predicate counter scoping): the evaluator/backend is intentionally tenant-scoped and shared across runs (rate/value caps keyed `(hook, tenant, capability)` with no run_id; a run-scoped limit would reset every run and enforce nothing). Document the split explicitly — per-run-fresh dispatcher, tenant-scoped predicate counters — in the module docs and fix the misleading "per-run isolation of hook state" wording in `ironclaw_reborn` (loop_driver_host.rs / runtime.rs). Add `predicate_counter_state_is_tenant_scoped_across_rebuilds`, which drives a real rate-cap predicate through two dispatchers from one factory and proves the second run sees the first run's recorded count. Rename `factory_mints_independent_dispatchers_per_call` -> `rebuild_mints_independent_dispatchers_per_call` and scope it to proving dispatcher freshness only. #6 (loop_driver_host tests): clarify that the hand-built builder factories cover host PLUMBING, not composition activation. Add `build_reborn_runtime_activates_hooks_through_real_composition_path`, which drives the real `build_reborn_runtime` with `HooksActivationConfig` threaded through `RebornRuntimeInput` (env-free) and the canonical registry, proving the production activation wiring composes. #2 (env boundary) and #5 (empty production catalog) were already fixed in 1e618d0; docs touched here for consistency. Known follow-up (not one of the six items, not introduced here): with the flag ON the standalone local-dev runtime does not yet reach `Completed` for a capability turn even with a zero-binding dispatcher — the composition root wires the dispatcher but not the companion hooked-prompt dependencies. The new runtime test asserts `is_terminal()` + the capability path and documents the gap. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(hooks): third-party hook-only projection core (flag, newtype, quarantine, caps) Steps 1-6 of third-party extension hook activation via hook-only projection: - Step 1: HOOKS_THIRD_PARTY_ENABLED sub-flag on HooksActivationConfig (default OFF; is_third_party_enabled() requires master flag too). Resolved at the CLI edge via from_env(). - Step 2: tenant_extension_root(&TenantId) derives the fixed /system/extensions/<tenant> root from identity (never caller-supplied); projection-layer strict-child / no-`..` containment check. - Step 3: build_hook_projection_registry assembles a HookProjectionRegistry (type-enforced hook-only newtype: no Deref / conversion back to ExtensionRegistry, so it can never reach HostRuntimeServices::new / the capability path). Sub-flag OFF => builtin-only, byte-identical to #3938. - Step 4/4a: atomic per-extension quarantine — untrusted (InstalledLocal) sets validated whole against a scratch builder, committed only if the whole set passes; any failure drops the extension's hooks entirely, emits a hook.quarantined security_audit tracing event (warn!, not info!), and continues. Trusted (HostBundled) sources stay fail-closed-whole-build. - Step 5: MAX_INSTALLED_EXTENSIONS_CONSIDERED / MAX_TOTAL_HOOKS_PER_TENANT DoS caps; count_total_bindings() accessor on HookDispatcher(Builder); pre-read MAX_MANIFEST_BYTES bound via read_file_bounded in discovery. - Step 6: third-party WASM stays out (loader registrar has no wasm_runtime) => WASM-bodied hook quarantines + build continues. Registrar-only invariant: projection installs go exclusively through HookRegistrar::install (ceiling + spoof-blocked owning_extension), never the direct builder installer API. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(hooks): FS-scoped tenant isolation (Option 1), trust matrix, registrar-only assertion Resolve the discovery/path conflict: the discovery layer hardcodes package roots to /system/extensions/<id> because the per-tenant RootFilesystem is the scope boundary (as with every other tenant-scoped resource), not a tenant path segment. So: - tenant_extension_root -> fixed /system/extensions (no tenant segment). The per-tenant RootFilesystem handed to discovery IS the isolation boundary. Documented as load-bearing; the openat2(RESOLVE_BENEATH)/O_NOFOLLOW backend hardening follow-up is what protects it (gating note kept prominent). - build_local_dev mounts /system/extensions to a per-owner host subtree under the storage root (per-identity by construction, not a process-global mount); exposed via RebornLocalRuntimeServices.extension_filesystem. - enforce_root_containment retained as defense-in-depth. Tests: - Integration (real build_hook_projection_registry + build_hook_dispatcher_ builder_factory through a fake RootFilesystem, not a loader look-alike): containment (hook present / capability absent by construction), FS-as-boundary tenant isolation proof (two distinct per-tenant filesystems; A can't see B), bad dir name skipped, id mismatch not a panic, surplus-extensions DoS cap, sub-flag OFF discovers nothing. - Per-hook-point trust matrix: BeforeCapability installed deny IS allowed and fires (Gate reachable); before_prompt predicate quarantined + build continues; after_model/after_capability/after_checkpoint/event_triggered WASM-only => quarantined + build continues; owning_extension derived (not spoofable). - Discovery pre-read bound: oversized manifest rejected via stat WITHOUT reading the body (fake fs panics on get); within-bound proceeds to read. - ironclaw_architecture source assertion: the hooks.rs projection path never calls install_installed_* directly (registrar-only invariant). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(hooks): correct Option-1 path-shape references in test comments Update the third-party projection integration-test module docs to reflect the FS-scoped isolation model (fixed /system/extensions root; per-tenant filesystem is the boundary), not the abandoned /system/extensions/<tenant> path segment. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks): tolerant+bounded third-party discovery, structural hook-only containment Addresses Codex P1/P2 + serrrfirat P1 on #3951. Critical 1 (discovery-stage DoS): add `ExtensionDiscovery::discover_with_manifest_contracts_tolerant_bounded` (+ `discover_extensions_tolerant_bounded` host-runtime wrapper). It lists+sorts the root once, then reads/parses at most `max_extensions` manifests, recording the surplus as quarantines WITHOUT reading them. The hook projection calls this with `MAX_INSTALLED_EXTENSIONS_CONSIDERED`, so the count cap fires before the per-manifest read storm. New all-or-nothing path delegates to a shared `load_package_entry` so per-package semantics are identical. Critical 2 (fail-open): tolerant discovery quarantines a single malformed/oversized/id-mismatched package and CONTINUES; valid siblings still load. The builtin-only fallback is now reserved solely for failure to LIST THE ROOT (directory unreadable). One bad manifest can no longer drop a tenant's entire legitimate third-party hook set. Refinement 3: the per-tenant hook budget is consumed only AFTER a successful merge, so a quarantined/duplicate package no longer burns budget. Refinement 4: the registrar-only arch assertion now scans the WHOLE composition crate (every non-test source) and forbids all installed-tier-minting primitives crate-wide (`install_installed_*`, `install_observer(`, `insert_binding(`, `HookTrustClass::Installed`) — not just a hooks.rs substring scan. Installed-tier bindings can only be minted via `HookRegistrar::install`. serrrfirat P1 (structural containment): `HookProjectionRegistry` no longer wraps `ExtensionRegistry`. It carries `Vec<HookProjection>` — hook metadata only (id/version/source/root/[[hooks]]). The projection literally cannot reach capabilities because it does not hold them; containment is by data shape, not a withheld conversion. Removes the `ExtensionPackageView` ceremony. Tests: bounded read-storm cap (read-counting fs panics on surplus), tolerant per-package quarantine, root-unreadable fallback, quarantined-package-does-not- consume-budget, malformed-sibling-survives at the projection layer. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hooks): address serrrfirat review — tenant-attributed install audits + hooks decomposition Addresses the maintainability review on #3951. Findings #1 (narrow hook-only boundary), #2 (per-extension discovery quarantine + mixed-batch test), and #5 (behavioral arch-test invariant) were already satisfied by the head commit (2b62597); this commit closes the two remaining items and hardens the arch test against the decomposition: - #3 (tenant attribution): add `build_hook_dispatcher_builder_factory_for_tenant`, threading the authenticated `tenant_id` (and its derived extension root) into the install-time quarantine-audit seam. `build_reborn_runtime` now calls it, so install-time quarantine audits carry the real tenant instead of the synthetic `reborn-hook-projection` fallback (closing the split where only discovery-time audits were attributed). New caller-driven test `for_tenant_entry_point_attributes_install_time_quarantine_to_real_tenant` asserts attribution via a deterministic thread-local audit capture (immune to tracing's process-wide max-level filter under parallel tests). - #4 (decomposition): split the 1.7k-line `hooks.rs` into a focused `hooks/` module — `mod.rs` (flag/config + public surface), `projection.rs` (hook-only `HookProjection`/`HookProjectionRegistry` containment + discovery/admission), `factory.rs` (first-party install, per-extension quarantine validation, fresh-per-build replay), `audit.rs` (`hook.quarantined` emission), and `tests.rs` (the test matrix). Behavior-preserving; no logic change. - arch test: skip dedicated test-module files in the registrar-only scan so the #4 decomposition cannot break it; the whole-crate behavioral invariant is preserved. - audit emission uses `debug!` (not `warn!`) per the background/hook-path logging rule, on the stable filterable `security_audit` target. - gemini #353: add the documented no-empty-segment guard to `enforce_root_containment` (defense-in-depth, not relying on VirtualPath canonicalization). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): cover hook-entry rejection branches; document InvocationCount cap semantics (#3938) Address henrypark133 review (review 4367870023): - Add extension-manifest tests for the three previously-uncovered hook-entry validation branches: non-table `[[hooks]]` element, whitespace-only `id`, and oversized entry (HookEntryTooLarge). - Document the InvocationCount inclusive-allow / deny-on-overflow semantics inline at the comparison site; behavior unchanged and still pinned by the cap test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(deps): pin kuchikikiki to 0.9.1 (0.9.2 yanked) cargo-deny failed on the yanked kuchikikiki 0.9.2 pulled in transitively via readabilityrs. Downgrade to 0.9.1 at the lockfile level. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(reborn-cli): assert hooks config threaded in caller test (#3938) Addresses the review finding that `build_runtime_input_maps_configured_cli_identity` exercised `build_runtime_input` but never asserted the `hooks` field, so a regression silently dropping `with_hooks_config(HooksActivationConfig::from_env())` or flipping the default-OFF rollout-safety contract would pass. Per `.claude/rules/testing.md` ("Test Through the Caller"): add two assertions to the existing caller-level test: - threading: `runtime_input.hooks == HooksActivationConfig::from_env()`, proving the env-resolved config is actually threaded through and not dropped. Verified via TDD that dropping the wiring fails this assertion (run under HOOKS_ENABLED=1). - default-OFF: when `HOOKS_ENABLED` is unset, `!runtime_input.hooks.is_enabled()`, guarded to skip if the CI environment exports the flag so it only pins the contract it claims to. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(hooks): cover build_reborn_runtime third-party wiring + async dir create (#3951) Address serrrfirat review findings M1 and L2. M1: add an integration test in tests/runtime.rs that drives build_reborn_runtime with HooksActivationConfig::enabled().with_third_party_enabled(true), a real /system/extensions manifest tree on the local-dev host filesystem, and tenant attribution. Asserts the runtime builds, starts a conversation turn, and shuts down cleanly — exercising the runtime.rs third-party discovery input + projection registry + tenant-threading wiring that was previously uncovered (the projection tests call build_hook_projection_registry / the dispatcher factory directly, and every other build_reborn_runtime call used the default disabled config). Verified the test fails when the wiring is broken. L2: switch the new factory.rs blocking std::fs::create_dir_all for the extensions host root to tokio::fs::create_dir_all(...).await with the same error mapping, so it no longer blocks the tokio executor thread inside the async build_local_dev. The two pre-existing std::fs calls (lines 132/136) are out of this PR's diff per the posted promise and are left untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(hooks): L1 quarantine-surfacing gate doc, L3 robust test-mod strip, M1 coverage-gap TODO Address serrrfirat 2026-06-03 review (M1/L2 already landed in 9866793). L1 (security observability): hook.quarantined audit events are emitted only via tracing at the security_audit target / debug! level, which production typically disables. Document durable quarantine surfacing as a hard production-enablement prerequisite for HOOKS_THIRD_PARTY_ENABLED, alongside the existing openat2(RESOLVE_BENEATH)/O_NOFOLLOW FS-hardening note, at all three gate doc sites: HooksActivationConfig (hooks/mod.rs), the runtime.rs composition -root gate comment, and the audit.rs module doc. L3 (robustness): strip_test_module matched #[cfg(test)]\nmod tests specifically and only the first occurrence. Generalize the anchor to #[cfg(test)]\nmod (any module name) so a refactor that renames the test module or adds a second #[cfg(test)] mod block is still fully stripped, preventing false positives in the FORBIDDEN_INSTALLED_PRIMITIVES architecture scan. M1 (test coverage): the build_reborn_runtime third-party wiring test already landed in tests/runtime.rs (9866793). Add the reviewer-requested TODO preserving the removed test's Cancelled-outcome coverage gap: the stub local-dev gateway cancels the turn before any capability dispatches, so the test exercises discovery + projection + tenant-threading at build/start but not end-to-end hook enforcement. NOTE: third-party discovery is intentionally tolerant (skips unparseable manifests), so this test catches compile-time field/arg regressions and build-path failures but not a silent manifest-read drop; documented for the reviewer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(hooks): correct in-memory backend warning text; delegate test ctor (#3938) Address serrrfirat review (2026-06-03): - Low: the in-memory backend warning claimed the LRU cap is shared across tenants, but the Reborn composition constructs a fresh InMemoryPredicateStateBackend per tenant. Rewrite the warn! text and doc comment so the real limitation (process-local replay dedup for multi-host deployments) is accurate, and note the backend is per-tenant in this composition. - Nit: PredicateEvaluator::with_backend (test-only) and with_state_backend had identical bodies; delegate with_backend to with_state_backend so they stay in lockstep. The Medium finding (hooks_config assertion in build_runtime_input caller test) was already addressed in 218a1de. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
nearai#3919) * feat(events): SecurityAuditSink primitive for boundary decisions Introduce a payload-free recording trait for security-boundary decisions (allow/block/scope-mismatch/replay-reject) emitted from defense-in-depth surfaces across IronClaw Reborn. Adopt it at the leak-detector / output redaction boundary inside `BuiltinObligationHandler::complete_dispatch` as a worked example. Motivation: across PR review on nearai#3573 (hooks), nearai#3767 (no-exposure guard), nearai#3888 (auth continuation), and nearai#3903 (credential boundary), security boundaries kept emitting their decisions via `tracing::warn!` / `tracing::error!`. Per the repo `CLAUDE.md` REPL/TUI rule those levels corrupt the interactive display and are forbidden for non-user-facing diagnostics. CLAUDE.md also says LLM data is never deleted — security-boundary decisions are exactly the class of data that should be retained, filterable, and never the only signal on a `warn!` line. The `SecurityAuditEvent` shape is payload-free by construction: there is no `String` "details"/"reason"/"message" field a careless caller could stuff a secret into. The struct carries only a `SecurityBoundary` enum, a `SecurityDecision` enum, an optional `CapabilityId`, an optional `ResourceScope`, a `SystemTime`, and a `&'static str` reason code that serves as a stable SRE grep target. New reason codes are expected to be added as `pub const` items in the module that owns the boundary. Sink contract is sync and best-effort: a sink failure must not change the outcome of the surrounding security decision (the boundary has already decided block/allow by the time it records). Provides `NoopSecurityAuditSink`, `TracingSecurityAuditSink` (emits at `debug!` — never `warn!` or `info!` — to respect the REPL rule), and `InMemorySecurityAuditSink` for tests. Adoption: `BuiltinObligationHandler` gains an optional security audit sink. When `redact_output` rejects output because the leak detector matched a BLOCK-class pattern, the handler records `SecurityAuditEvent { boundary: LeakDetector, decision: Blocked, code: "leak_redact_failed" }` before propagating the error. Test drives `complete_dispatch` (not the helper) per the "Test Through the Caller" rule in `.claude/rules/testing.md`. Out of scope for this PR (each is its own follow-up): - `RebornProductWorkflowAuthContinuationDispatcher` `warn!` → audit - MCP direct-lease deny path - Hook predicate / envelope deny paths - Credential-channel boundary blocks - NoExposureGuard egress blocks (PR nearai#3767 follow-up) * ci: fmt + clippy + no-panics, address henrypark M1 - Annotate three InMemorySecurityAuditSink mutex .expect() calls in crates/ironclaw_events/src/security_audit.rs with `// safety:` to satisfy the no-panics scanner. This is a test-only sink; a poisoned mutex would only mean an earlier test thread already panicked, which we explicitly want to surface. - Promote LEAK_REDACT_FAILED_CODE to `pub` and re-export from ironclaw_host_runtime::lib so downstream crates can match on the stable code (addresses henrypark M1 review note).
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…earai#3573) * feat(reborn): add ironclaw_hooks framework foundation (#3524) Foundation slice of the Reborn loop hooks framework per nearai/ironclaw#3524. Lands the trust primitives, sealed decision types, dispatcher contract, and extension manifest schema; no Reborn middleware composition yet (next slice wires HookDispatcher into LoopCapabilityPort / LoopPromptPort). Design comment on #3524: https://github.com/nearai/ironclaw/issues/3524#issuecomment-4439890144 What this PR ships ================== * `crates/ironclaw_hooks/` — new crate * `identity` — content-addressed `HookId` (blake3 of length-prefixed extension + local + version fields). Same versioning primitive the rest of Reborn should converge on for replay safety. * `trust` — `HookTrustClass` enum (Builtin / Trusted / Installed) with per-kind default attenuation. Trust class is fixed by source, never declarable. * `kinds/` — sealed decision DTOs. `BeforeCapabilityHookDecision`, `HookPatch`, `ObserverFact` all have `pub` outer struct + `pub(crate)` inner enum + `pub(crate)` constructors. Same #3460 witness pattern. * `points/` — typed read-only contexts for each hook point. * `sink` — split sink traits per trust tier. `PrivilegedGateSink` exposes `allow()`; `RestrictedGateSink` does not. An Installed-tier hook literally cannot mint Allow at the type level. * `ordering` — phase → priority → hook id, stable. Phases gated by trust (Validation/Authorization Builtin-only). * `failure_policy` — Timeout/Panic/Malformed/AttenuationViolation categories. Gate/Mutator fail closed, Observer/Effect fail isolated. Slot poisoning persisted for the rest of the run on any category. * `registry` — run-profile-sourced bindings; phase-vs-trust gate enforced at insert; poisoning surface for the dispatcher. * `dispatch` — HookDispatcher with deterministic ordering, panic catch-unwind via futures::FutureExt, per-hook tokio::time::timeout, short-circuit gate composition (Deny > PauseAuth > PauseApproval > Allow), Telemetry-phase observers always run. * `manifest` — serde types for the `[[hooks]]` section of extension manifests. Predicate vs WASM body; same_tenant scope requires explicit grant; Validation/Authorization phases rejected at parse time because manifest hooks are always Installed. * `predicate` — typed predicate language for declarative Installed hooks (DenyCapability, PauseApproval, RateOrValueCap). Evaluator lives in the dispatcher follow-up, not here. * `crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs` * Added `ironclaw_turns` -> `ironclaw_hooks` to the forbidden list. * New BoundaryRule for `ironclaw_hooks` itself (cannot pull host_runtime, dispatcher, secrets, network, wasm, etc.). * `Cargo.toml` workspace member registration. What this PR deliberately does NOT ship ======================================== * Reborn middleware composition wrapping LoopCapabilityPort / LoopPromptPort with HookDispatcher. Next slice; ironclaw_reborn changes only. * WASM hook execution path. Programmatic hooks parse and validate from manifest; the wasmtime integration lands when the WASM dispatcher seam is built. * Predicate evaluation. Predicate types serialize and validate; the evaluator that turns a `RateOrValueCap` spec into a `Deny` decision is in the next slice alongside Reborn wiring. * Event-triggered hooks (Phase 5 of the original roadmap). * Self-authored hooks. Tracked separately at #3567 with monotonic-restriction + unforgeable-channel ratification. Test plan ========= * `cargo test -p ironclaw_hooks` — 47 tests (46 unit + 1 integration smoke for the manifest -> binding -> dispatch pipeline). * `cargo test -p ironclaw_architecture` — 13 tests; new boundary rule passes, existing rules unaffected. * `cargo clippy -p ironclaw_hooks --all-targets -- -D warnings` — clean. * `cargo fmt -p ironclaw_hooks -- --check` — clean. * `cargo check --workspace` — clean, no regressions in other crates. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(reborn): wire HookDispatcher into LoopCapabilityPort/LoopPromptPort Follows the foundation slice (see initial commit). Adds the next layer: 1. Capability- and prompt-port middleware (`ironclaw_hooks::middleware`) * `HookedLoopCapabilityPort` runs `dispatch_before_capability` before every invocation, translates the composed decision into the existing `CapabilityOutcome` vocabulary (Deny / PauseApproval / PauseAuth all map to `Denied` for now; gate-ref plumbing for real pause semantics lands in the next slice). * `HookedLoopPromptPort` runs `dispatch_before_prompt` before bundle construction. Observe-only for snippets in this slice; actual snippet injection waits for the shared `prompt_envelope::wrap_untrusted` helper (#3540 / #3471). 2. Declarative predicate evaluator (`ironclaw_hooks::evaluator`) * `DenyCapability` and `PauseApproval` predicates: stateless, evaluated directly against `BeforeCapabilityHookContext`. * `RateOrValueCap` with `InvocationCount` bound: sliding-window counter keyed by `(hook_id, capability_name)`, in-memory only. Window parsing supports `s`/`m`/`h`/`d` units; unparseable windows fail closed. * `NumericSum` bound: types implemented but evaluation returns Allow and emits a warn-level audit. Full argument-extraction story is a follow-up slice once capability arguments become hook-visible. * `PredicateEvaluator::evaluate_at(...)` test variant accepts an explicit `Instant` so sliding-window tests don't depend on real-clock progress. 3. Manifest -> dispatcher glue (`ironclaw_hooks::installed_hook`) * `PredicateBackedBeforeCapabilityHook` wraps a `HookPredicateSpec` plus an `Arc<PredicateEvaluator>` and implements `RestrictedBeforeCapabilityHook`. The registry installer would construct one of these per `[[hooks]]` entry whose body is `HookManifestBody::Predicate`. * Sink reasons are `&'static str`, so the dynamic predicate `reason` surfaces in audit (via the evaluator's `EvaluatorDecision`) rather than the model-visible decision. Closed-vocabulary labels carry through to the sink. 4. Reborn composition seam (`ironclaw_reborn::loop_driver_host`) * `RebornLoopDriverHostFactory::with_hook_dispatcher(Arc<HookDispatcher>)` opt-in builder method. When set, the factory wraps the capability and prompt ports with the hooked middleware. Default behavior (no dispatcher) is unchanged from the pre-hooks shape, so existing callers continue to work. * Added `ironclaw_hooks` as a dep in `ironclaw_reborn`. Test plan ========= * `cargo test -p ironclaw_hooks` — 60 tests pass (59 unit + 1 integration smoke; +13 vs the foundation commit covering middleware, evaluator, installed_hook). * `cargo test -p ironclaw_reborn` — 118 tests pass; no regressions from adding the dep. * `cargo test -p ironclaw_architecture` — 13 tests pass; the `ironclaw_turns -> ironclaw_hooks` boundary still holds and the new `ironclaw_hooks` rule (no host_runtime / dispatcher / secrets / network / wasm / reborn) is unaffected. * `cargo clippy -p ironclaw_hooks --all-targets --all-features -- -D warnings` — clean. * `cargo clippy -p ironclaw_reborn --all-targets -- -D warnings` — clean. * `cargo fmt --all -- --check` — clean. What still defers ================== * WASM hook execution path. * Persistent predicate counter (in-memory only for now). * Argument-extraction so `NumericSum` predicates evaluate against capability arguments. * Gate-ref plumbing so PauseApproval / PauseAuth surface real `CapabilityOutcome::ApprovalRequired` instead of `Denied`. * Prompt-snippet injection (waits for shared envelope helper). * Event-triggered hooks. * Self-authored hooks (#3567). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(reborn): add HookedLoopModelPort/TranscriptPort/CheckpointPort observer middleware Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(reborn): end-to-end hooks integration through RebornLoopDriverHostFactory Adds crates/ironclaw_reborn/tests/hooks_integration.rs covering the factory's HookDispatcher wiring seam end-to-end. Tests drive host.invoke_capability(...) (not dispatcher.dispatch_before_capability(...) directly) so a regression in RebornLoopDriverHostFactory's wrapping composition surfaces here. Scenarios: - PredicateBackedBeforeCapabilityHook (DenyCapability NameEquals "cap.blocked") short-circuits invocation; inner port never called; outcome is Denied(unknown("hook_denied")). - A privileged selective hook that allows non-matching capabilities proves the wrapper does not blanket-deny: cap.allowed reaches the inner port and completes once. - Factory built without with_hook_dispatcher() lets cap.blocked through to the inner port, proving the hook plumbing is genuinely opt-in. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(reborn): add pass() + HookRegistrar + self-authored hooks scaffolding Three additions to ironclaw_hooks: B. `pass()` on gate sinks — distinguishes "evaluated, no opinion" from "returned without minting a decision." A passing hook contributes nothing to the composed decision; a silent hook is still Malformed and fails closed. `PredicateBackedBeforeCapabilityHook` now routes the evaluator's `Allow` decision through `sink.pass()` instead of the previous `deny("hook_predicate_pass")` workaround. A. `HookRegistrar` bridge — converts a `Vec<HookManifestEntry>` into `HookBinding`s + dispatcher impls in one call. Predicate bodies are wired through `PredicateBackedBeforeCapabilityHook`; WASM bodies return `HookError::RegistryConstruction` for now. Adds `HookDispatcher::insert_binding` so the registrar can mutate the registry through the dispatcher rather than reach inside. I. Self-authored hooks scaffolding — fourth `HookTrustClass` variant for hooks the agent authors at runtime. Run-scoped only; monotonic-restriction sink with no `allow`, no trusted-snippet path, no effect-class constructor. Closed-vocabulary `SelfAuthoredReason` enum keeps free-text reasons off the audit seam. `SelfAuthorshipProvenance` captures authoring run/turn, timestamp, spec digest, optional user ratification, and a generation-trace pointer. Durable persistence depends on the unforgeable channel from #3564 and lands separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(reborn): real gate-ref plumbing for hook PauseApproval/PauseAuth decisions Previously, `GateDecisionInner::PauseApproval` and `PauseAuth` returned by hooks were degraded to `CapabilityOutcome::Denied` at the middleware boundary because the hook crate had no way to mint a `LoopGateRef` scoped to the current run. Hooks that wanted to pause the loop for approval or auth instead failed the call closed, leaving the host's approval-router machinery unreachable from hook code. This change introduces a `HookGateRefFactory` trait in `ironclaw_hooks::middleware::gate_ref` that mints `LoopGateRef`s for pause-class decisions. `HookedLoopCapabilityPort` now takes an `Arc<dyn HookGateRefFactory>`, defaulting to `UuidHookGateRefFactory` (a locally-unique opaque-id factory suitable for tests and the foundation slice). Production deployments override via `.with_gate_ref_factory(...)` with a factory bound to the current `LoopRunContext` and the host's gate-router. The translation in `decision_to_outcome` is now async so it can await the factory. `PauseApproval` maps to `CapabilityOutcome::ApprovalRequired { gate_ref, safe_summary }` and `PauseAuth` to `AuthRequired`. If the factory itself errors, the middleware falls back to `Denied` with a sanitized `hook_gate_ref_unavailable` reason kind so the loop fails closed rather than routing through an unresolvable suspension. The underlying error text is dropped to avoid leaking gate-router state into model-visible output. Tests: - `pause_approval_decision_surfaces_as_approval_required`, `pause_auth_decision_surfaces_as_auth_required`, `gate_ref_factory_failure_falls_back_to_denied` in `middleware::capability_port::tests`. - `pause_approval_hook_surfaces_as_approval_required_with_real_gate_ref` in `crates/ironclaw_reborn/tests/hooks_integration.rs`, exercising the full `RebornLoopDriverHostFactory` composition with the default `UuidHookGateRefFactory`. - Gate-ref factory unit tests in `gate_ref::tests`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(reborn): add NumericSum predicate evaluation with capability argument extraction Wires the missing argument-extraction story for the predicate evaluator so `ValueOrRateBound::NumericSum` actually enforces a rolling numeric cap instead of warn-and-allowing. - Extend `BeforeCapabilityHookContext` with a sealed `SanitizedArguments` view. Strings truncate to 256 bytes; objects/arrays cap at 8-deep. `extract_numeric` supports dotted + bracketed paths (`order.amount`, `items[0].price`) and returns `Option<rust_decimal::Decimal>`. The inner representation is sealed so external callers can't bypass bounds. - Introduce `CapabilityInputResolver` + bundled `NullCapabilityInputResolver` in `middleware/resolver.rs`. The hooks crate intentionally doesn't know how to dereference a `CapabilityInputRef` — that knowledge belongs to the production host. Until a real resolver is wired in (follow-up), arguments are `Unresolved` and `NumericSum` fails closed. - `HookedLoopCapabilityPort::new` defaults to the null resolver; new builder `.with_resolver(Arc<dyn CapabilityInputResolver>)` overrides. - `PredicateEvaluator` gains a tenant-keyed `value_history` map. The `NumericSum` arm parses `max` + `window`, extracts the numeric value from sanitized args, accumulates within the rolling window, and applies `on_exceeded` when the sum exceeds the cap. Unresolved args, missing field, non-numeric field, unparseable max, and unparseable window all fail closed via the configured `OnExceededAction`. - Add `BeforeCapabilityHookContext::new_unresolved(...)` convenience ctor; existing test sites switch to it instead of churning every call site through the 4-arg ctor. Test count: +14 (8 new SanitizedArguments tests, 6 new NumericSum evaluator tests, 1 null-resolver test; one old NumericSum-stub-related gap closed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(reborn): seal hook registration trust boundary + dispatcher hardening Addresses blocking findings from the security audit of `ironclaw_hooks`: - C1 (Blocking, Trust Model): "Installed cannot Allow" was not enforced at the registration boundary. `BeforeCapabilityHookImpl::Privileged` was a public variant, so external crates with dispatcher access could construct an Installed binding paired with a Privileged impl and bypass the sink trait restriction. Sealed `BeforeCapabilityHookImpl`, `BeforePromptHookImpl`, and `ObserverHookImpl` to `pub(crate)` and replaced the single generic `install_before_capability` / `install_before_prompt` / `install_observer` surface with tier-specific public installers (`install_builtin_*`, `install_trusted_*`, `install_installed_*`) that build the binding with the matching trust class internally. Updated registrar, internal middleware tests, the hooks foundation pipeline test, and the reborn `hooks_integration` test to drive the new surface. Added regression tests proving the trust class is set by the installer and that the seal is type-level. - C5 (Medium, Slot Poisoning): same-dispatch poisoning was incomplete because `ordered_bindings` snapshots once at the top of the loop, and `HookRegistry::insert` accepted duplicate hook IDs. Rejected duplicate hook IDs (any point) in `HookRegistry::insert` and added a poison re-check before invoking each hook impl in `dispatch_before_capability`, `dispatch_before_prompt`, and `dispatch_observer_at`. Added regression tests for both behaviors. - C6 (Medium, Manifest / Predicate Validation): `parse_window` could panic on non-ASCII input because `split_at(len - 1)` requires a char boundary. Rewrote to compute the unit char's UTF-8 byte length and slice safely, added a public `validate_window` helper, and wired it into `HookManifestEntry::validate` for both `InvocationCount` and `NumericSum` bounds. Added tests for non-ASCII, empty, single-char, and zero-duration windows. - C2 (High, Tenant Isolation): partial fix only. The `PredicateEvaluator`'s sliding-window counter was keyed by `(hook_id, capability)`, so cross-tenant state could leak. Extended `HistoryKey` to include `tenant_id` and added a regression test proving counters partition by tenant. Documented the broader dispatcher-per-build / per-run-fresh-dispatcher pattern as deferred follow-up in `crates/ironclaw_hooks/CLAUDE.md`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(reborn): emit hook telemetry milestones for audit/SSE observers Wires the hook dispatcher into the host's milestone stream so audit backends and SSE observers can see hook activity. Previously, hook dispatch was invisible — denies, pauses, failures, and observer fires left no trace in the host's observability backend. Changes: - `ironclaw_turns`: add `HookDispatched`, `HookDecisionEmitted`, and `HookFailed` variants to `LoopHostMilestoneKind`, with a closed- vocabulary `HookDecisionSummary` enum (Allow/Deny/PauseApproval/ PauseAuth/Pass/Patch). Introduce a lightweight `HookMilestoneSink` trait that emits hook-specific *kinds* without requiring a `LoopRunContext` (the dispatcher is a process-wide singleton that cannot own a per-run context), plus a `RunScopedHookMilestoneSink` adapter that injects run context and forwards to the existing `LoopHostMilestoneSink`. Also add `InMemoryHookMilestoneSink` for tests. - `ironclaw_hooks`: add a `telemetry` module that converts hook-crate types (`HookId`, `HookTrustClass`, `HookPointSpec`, `FailureCategory`, `FailureDisposition`, `BeforeCapabilityHookDecision`) into the wire- shape labels and summaries the milestone sink expects. Hook ids cross the seam as hex strings because the strongly-typed `HookId` cannot be imported from `ironclaw_turns` (the architecture test enforces `ironclaw_turns -> ironclaw_hooks` stays absent). - `ironclaw_hooks::dispatch`: add an optional `Arc<dyn HookMilestoneSink>` to `HookDispatcher`, set via `with_milestone_sink`. Emit `HookDispatched` before each hook runs, `HookDecisionEmitted` after a decision/pass/patch, and `HookFailed` on timeout/panic/malformed/missing-impl across all three dispatch paths (before_capability, before_prompt, observer). Default behavior (no sink attached) emits nothing — preserves the pre-telemetry observable surface. - `ironclaw_reborn`: document on `with_hook_dispatcher` that callers attach the milestone sink to the dispatcher *before* wrapping it in `Arc` and installing it into the factory, using a `RunScopedHookMilestoneSink` to inject run-context. The dispatcher itself is shared across runs, so attaching a fixed run-context inside it would be wrong. Update `RuntimeEvent` projection in `milestone_events.rs` to ignore the new hook kinds (no projection pathway yet; emitted milestones are consumed by SSE observers directly). Tests: - `ironclaw_hooks::dispatch`: 5 new tests covering milestone emission for deny decisions, panic failures, prompt-mutator patches, observer pass-throughs, and the no-sink default. - `ironclaw_reborn` hooks_integration: end-to-end test wiring a `RunScopedHookMilestoneSink` onto the dispatcher and asserting hook activity surfaces in the host's `LoopHostMilestoneSink`. Total: +6 hook telemetry tests; no existing tests modified. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(reborn): extract shared prompt envelope; inject hook patches into prompt bundle Adds `ironclaw_prompt_envelope`, a leaf crate that owns the single envelope primitive used by every model-visible untrusted-content path. `wrap_untrusted` prefixes content with a closed-vocabulary `<Trusted|Untrusted> <source> content: ` marker, rejects bodies carrying instruction-hijack phrases (`ignore previous instructions`, `<|im_start|>`, `<system>`, etc.), and enforces a 4 KiB byte budget by default. Migrates `ironclaw_host_runtime::memory_context` to delegate envelope wrapping, marker rejection, and control-character stripping to the new crate while keeping the `LoopSafeSummary`-specific 512-byte cap and byte truncation local. Existing memory_context behavior and tests are preserved. Wires the same envelope into `ironclaw_hooks`: * `HookPatch::add_enveloped_snippet` now takes a raw body and wraps it via `wrap_untrusted(EnvelopeSource::Hook, …)`. `Installed` hooks produce `Untrusted` envelopes; `Builtin`/`Trusted`/`SelfAuthored` produce `Trusted` envelopes so downstream readers can distinguish the two paths through a uniform marker. * `HookedLoopPromptPort::build_prompt_bundle` is no longer observe-only. After dispatching `before_prompt`, it envelope-wraps every snippet patch (passing `Enveloped` through, wrapping `Trusted` with the envelope helper), enforces the 4 KiB aggregate snippet byte budget across patches, and appends the wrapped snippets to the prompt bundle's `messages` as `system`-role `LoopModelMessage` entries carrying deterministic `msg:hook.<ordinal>.<hash>` content refs (mirroring the skill-snippet ref convention). The envelope crate is a leaf with no ironclaw dependencies, satisfying the boundary contract; the existing `ironclaw_hooks` boundary rule in `reborn_dependency_boundaries` continues to hold because `ironclaw_prompt_envelope` is not on its forbidden list. Test count delta: * `ironclaw_prompt_envelope`: +13 new tests (crate did not exist). * `ironclaw_hooks`: 84 → 88 tests (+4 prompt-port behavior tests: `hook_patch_appended_as_envelope_wrapped_message`, `total_byte_budget_enforced_across_patches`, `instruction_hijack_in_patch_rejected`, `trusted_hook_patch_wrapped_with_trust_marker`). * `ironclaw_host_runtime` memory_context: unchanged (8 tests still pass). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: align tenant-counter test with SanitizedArguments-extended context ctor * docs(reborn): document loader contract; pin HookId hex format Add a "Loader responsibility" section to ironclaw_hooks/CLAUDE.md explaining that tier-specific installers prevent minting wrong-tier impls but cannot enforce origin — that's the loader's job — and recommending registry loaders type-tag extension hooks as LoadedHook::Installed at the loader seam. Add tier_specific_installers_are_documented_as_loader_contract as a regression guard that touches every public install_*_before_capability and install_*_before_prompt method so any signature change forces the loader contract to be re-evaluated. Document HookId::to_hex's 64-char lowercase hex output as part of the cross-crate contract consumed by LoopHostMilestoneKind::Hook* in ironclaw_turns; add hook_id_hex_format_is_stable_64_lowercase_chars in identity::tests and hook_id_string_serialization_matches_to_hex in telemetry::tests to pin the format and the seam conversion path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(reborn): pin hook milestone JSON schema + assert pairing invariants Add L3 schema-snapshot tests for every hook-related LoopHostMilestoneKind variant (HookDispatched, HookDecisionEmitted per HookDecisionSummary, HookFailed per FailureCategory) so downstream consumers can rely on the JSON wire shape and any accidental field rename, enum-tag rename, or type change fails loudly. Add L4 pairing-invariant matrix test in the hook dispatcher that drives every observable outcome (Allow, Deny, PauseApproval, PauseAuth, Pass, Panic, Timeout, Malformed, MissingImpl) through a recording milestone sink and asserts the dispatched-then-terminator pairing shape. Document the MissingImpl path as the one case that emits a sole HookFailed with no preceding HookDispatched (the dispatcher discovers the protocol violation before the hook is actually dispatched). Add a multi-hook dispatch test that installs three hooks with mixed outcomes (allow/deny/panic) at the same point and asserts each hook produces its own paired sequence in the deterministic (phase, priority, hook_id) order taken from the dispatcher's registry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(reborn): integration tests for observer middleware through RebornLoopDriverHostFactory Wire the HookedLoopModelPort / HookedLoopTranscriptPort / HookedLoopCheckpointPort observer wrappers into RebornLoopDriverHostFactory::build_text_only_host_with_capabilities, mirroring the existing HookedLoopCapabilityPort / HookedLoopPromptPort composition. The wrappers are applied only when a HookDispatcher is set on the factory, so the default factory shape is unchanged. Add four integration scenarios in crates/ironclaw_reborn/tests/hooks_integration.rs: - observer_hook_fires_after_model_through_factory - observer_hook_fires_after_capability_through_factory - observer_hook_fires_after_checkpoint_through_factory - observer_panic_does_not_fail_model_call (panic-isolation regression) Relax the test-fixture model gateway from "panic if invoked" to returning a stub assistant reply so the AfterModel / panic-isolation tests can drive stream_model through the wrapped port. The existing capability-port tests never touch the gateway, so their behavior is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(reborn): introduce HookDispatcherBuilder for type-enforced sink wiring Adds a `HookDispatcherBuilder` in `ironclaw_hooks::dispatch` that owns the dispatcher construction lifecycle: registry -> optional timeout -> optional milestone sink -> installed hooks -> `.build_arc()`. The terminal `.build_arc()` wraps in `Arc` and yields an immutable handle. Tightens the public surface on `HookDispatcher`: `new`, `with_timeout`, `with_milestone_sink`, and every `install_*_*` method are now `pub(crate)`. Outside callers route exclusively through the builder, so "wire the milestone sink before Arc-wrapping" is a compile-time fact rather than a documentation convention. `HookRegistrar::install` now takes a `HookDispatcherBuilder` by value and returns `(HookDispatcherBuilder, Vec<HookId>)`, keeping the builder chainable through manifest installation. `RebornLoopDriverHostFactory` gains `with_hook_dispatcher_builder` to let callers defer `.build_arc()` to the factory — a step toward the FU8 per-build dispatcher pattern. Migrates `foundation_pipeline.rs` and `hooks_integration.rs` to the builder. Internal middleware and dispatch tests continue to use the crate-private `HookDispatcher::new` directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(reborn): production CapabilityInputResolver for NumericSum predicates Adds HookCapabilityInputResolverAdapter in ironclaw_reborn that bridges the existing LoopCapabilityInputResolver (already used by HostRuntimeLoopCapabilityPort for dispatch input resolution) to the hooks crate's CapabilityInputResolver trait. RebornLoopDriverHostFactory gains with_capability_input_resolver(...), and when both a hook dispatcher and resolver are configured the factory threads the adapter into HookedLoopCapabilityPort::with_resolver — so NumericSum and other argument-dependent predicates evaluate against real, sanitized inputs instead of failing closed against the framework's null default. The adapter also enforces a configurable serialized-byte budget (default 64 KiB) as defense in depth ahead of the hooks crate's per-string and depth caps in SanitizedArguments. Unit tests cover the four adapter branches (resolved JSON, inner-error → None, non-object pass-through, oversized → None) and a new end-to-end integration test (numeric_sum_predicate_caps_total_value_against_real_inputs) drives the full factory wiring: with a NumericSum cap of 99 over an "amount" field, two invocations carrying {"amount":"50"} let the first pass through and deny the second at the hook seam, with the inner port reached exactly once. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(reborn): per-build HookDispatcher for full per-run isolation (C2) Introduce `with_hook_dispatcher_factory(F)` on `RebornLoopDriverHostFactory`. The closure is invoked once per `build_text_only_host*` call, so dispatcher-owned mutable state — slot poisoning, registry mutations, predicate-counter siblings — is scoped to a single host build instead of shared across every host the factory produces. The legacy `with_hook_dispatcher(Arc<HookDispatcher>)` adapter is kept as a thin wrapper that returns clones of the same `Arc` on every build. Its shared-state behavior is now documented as an explicit opt-in for backward compat; new wiring should prefer the factory closure. Adds two regression tests: - `per_build_dispatcher_state_does_not_leak_across_runs` — installs a panicking hook, builds two hosts back-to-back, and proves the inner port is never reached on build 2 (fresh slot still applies the fail-closed deny). Pins per-run isolation. - `legacy_with_hook_dispatcher_shares_state_across_builds` — pins the shared-state semantic of the legacy adapter as the explicit baseline. Migrates `predicate_deny_hook_short_circuits_inner_port` to the new factory-closure path so the new wiring is exercised by the existing suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(reborn): project hook telemetry milestones into RuntimeEvent for durable audit Extend the runtime event substrate with `HookDispatched`, `HookDecisionEmitted`, and `HookFailed` kinds carrying closed-vocabulary labels and the blake3-hex hook identity. Project the matching `LoopHostMilestoneKind::Hook*` variants in `DurableLoopHostMilestoneSink` so hook telemetry now lands in the same durable event log as model/reply/loop milestones — SSE observers still see live hook events, and audit replay can reconstruct the full hook trail. - `ironclaw_events`: add hook variants to `RuntimeEventKind`, optional hook fields on `RuntimeEvent` (`hook_id`, `hook_point`, `hook_trust_class`, `hook_decision`, `hook_failure_category`, `hook_failure_disposition`), typed constructors (`hook_dispatched`, `hook_decision_emitted`, `hook_failed`), and dedicated sanitizers (`sanitize_hook_label`, `sanitize_hook_id`) re-run on every wire crossing. No new crate dependency edges; hook strings cross the boundary opaque. - `ironclaw_reborn::milestone_events`: project the three hook milestone kinds via a new `loop.hook` capability id. `HookDecisionSummary` is collapsed to its closed-vocabulary `kind_name()` so sanitized reasons never enter the durable substrate. - `ironclaw_event_projections`: extend `TimelineEntryKind` and the `RuntimeEventKind -> RunProjectionStatus` mapping so hook events are pure telemetry — they preserve the current run status rather than changing it. - Tests: 4 unit tests in `ironclaw_events::runtime_event::tests` (serde round-trip per variant + unsafe-label collapse), 3 in `ironclaw_reborn::milestone_events::tests` (projection per variant, including the assertion that raw `Deny { reason }` text does not reach the durable wire payload). Existing replay-projection direct-construction tests updated for the new RuntimeEvent fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(reborn): enforce manifest-declared hook scope at dispatch time (C3) Audit finding C3: extensions could declare `[[hooks]]` with `scope = "own_capabilities"` in their manifest, but the dispatcher never enforced it — an Installed hook from ext-A could fire against capabilities provided by ext-B. Scope was parsed but not load-bearing. This change makes scope load-bearing end-to-end: - `BeforeCapabilityHookContext` carries an optional `provider: ironclaw_host_api::ExtensionId` populated by the middleware. The hook context is `#[non_exhaustive]` already so this is non-breaking. - `HookBinding` gains `owning_extension: Option<ExtensionId>` and `scope: HookBindingScope`. `HookBindingScope` is `Global` / `OwnCapabilities` / `SameTenant`. Builtin and Trusted bindings default to `Global` and carry no `owning_extension`; Installed bindings carry both, sourced from the manifest. - `HookDispatcher::install_installed_*` installers now require the caller to pass `(owning_extension, scope)`. The registrar derives both from the manifest entry, so manifest authorship is the single source of truth. - A new `CapabilityProviderResolver` trait + bundled `NullCapabilityProviderResolver` lets the middleware lift the capability id to its provider at invocation time. The middleware wires the resolved provider into the hook context. - `dispatch_before_capability` consults `binding.scope.permits(...)` before invoking each hook. Bindings that don't permit the current invocation are inert — no sink call, no failure record, no poisoning. Conservative defaults: - When the provider resolver returns `None` (no resolver wired, or the capability has no known provider), `OwnCapabilities`-scoped hooks do NOT fire. An attacker cannot bypass scope filtering by stripping provider info from the descriptor. Tests: - 5 new dispatcher tests cover OwnCapabilities matching, foreign provider, unresolved provider, SameTenant, and Builtin Global. - 1 new registrar test asserts manifest scope and extension propagate into `HookBinding`. - 1 new middleware test asserts the provider resolver populates the hook context. - 1 new integration test in `ironclaw_reborn` proves an ext-A hook scoped to `OwnCapabilities` does not intercept invocations that have no resolved provider (the production composition default). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style: rustfmt dispatch.rs after FU1 merge * docs(hooks): prior-art comparison against LSM/eBPF/Envoy/K8s/OPA/CRX/VSC/Tauri Validates the IronClaw hooks design against 8 established hook/policy systems across 8 axes (dispatch, trust tiers, attenuation, decision vocabulary, failure semantics, isolation, manifest, audit). Surfaces: - 7 areas where ICLAW stands out vs prior art (type-level trust enforcement, dispatch-time scope, failure-kind matrix, pause-with- gate-ref, pairing-invariant audit matrix, tenant-keyed predicates, phase-ordered dispatch) - 4 conventional choices we should revisit (in-process Installed-WASM, sticky poison, no formal dispatch model, no installation rate-limit) - 3 divergences whose 'why' is weak and need design review * docs(hooks): STRIDE threat model for v1 framework Enumerates 7 adversary classes (A1-A7), 6 assets ranked by blast radius, and ~35 attack vectors across STRIDE categories with mitigations, existing tests, and residual risk. Surfaces 7 prioritized follow-ups: - High: per-extension hook-count cap (D3/D4) - High: gate-ref unguessability + one-shot test (S1) - Med: resolver field-level scope (I2) - Med: per-evaluator state ceiling (D5) - Med: poison-stickiness operator runbook - Low: timing side-channel residual acknowledgement (I4) - Low: instruction-marker denylist periodic review (I5) Confirms the load-bearing 'Installed cannot Allow' (E1) property holds via type-level seal + tier-specific installers, backed by compile_time_seal_test and installed_binding_cannot_be_paired_with_ privileged_impl tests. Explicit out-of-scope: extension install pipeline (#3492), WASM exec sandbox (needs separate threat model when it lands), approval gateway (#3564). * feat(hooks): close threat-model gaps S1 (gate-ref entropy) and D3/D4 (registration flood) S1 (gate-ref unguessability, factory side): - Three new tests on `UuidHookGateRefFactory`: - `gate_refs_are_v4_uuids` pins the v4 entropy source (122 random bits per ref per RFC 4122 §4.4); fails if a future change moves to a counter or weaker UUID version. - `gate_refs_have_no_collisions_across_many_calls` mints 20k refs across both namespaces, asserts zero collisions (statistical proxy for entropy quality). - `approval_and_auth_namespaces_do_not_overlap` confirms prefix routing separation. - Doc comment now documents the security property explicitly and delineates factory-side vs gateway-side responsibilities for the one-shot consumption property. D3/D4 (hook registration flood): - New `MAX_HOOKS_PER_EXTENSION = 32` and `MAX_HOOKS_PER_EXTENSION_PER_KIND = 8` consts in `registrar.rs`. - New `HookRegistrar::enforce_registration_caps` runs pre-flight at the top of `install()`, before any binding is inserted. Whole-batch rejection means a partially-installed batch cannot slip past. - Three regression tests: total-cap rejection, per-kind-cap rejection, at-cap acceptance. - Error messages cite the threat-model finding so operators can map rejection back to the design rationale. Threat model updated: S1, D3, D4 marked closed in the cross-cutting properties matrix and the open-follow-ups list. * test(hooks): three real hooks built against the public API + ergonomics findings Builds three representative hooks from outside the crate, mimicking what an extension or system author would actually write: 1. polymarket-daily-cap — Installed predicate hook, InvocationCount rate-cap with Deny on excess. Canonical 'rate-limit a capability' use case for the predicate language. 2. large-stake-approval-gate — Installed predicate hook, NumericSum over amount_usd field, PauseApproval at $1000/24h. Manifest-shape + registrar-install coverage from outside Reborn; end-to-end dispatch lives in ironclaw_reborn integration tests because NumericSum needs resolved args (a friction finding documented in the companion doc). 3. pii-redaction-warning — Trusted Rust hook implementing PrivilegedBeforePromptHook, injects a trusted instruction snippet reminding the model to redact PII. Demonstrates the path a system author takes when the predicate language isn't expressive enough. API change (F1 fix): SanitizedArguments::unresolved() promoted from pub(crate) to pub. This is the documented safe default — predicates that need args must fail closed against it — so exposing the constructor cannot weaken any trust property. The sanitizing from_json constructor stays sealed; that's the trust boundary. Without this fix, external hook authors could not construct a BeforeCapabilityHookContext with both a known provider AND unresolved args, which made TDD of their own predicate impossible. Findings documented in docs/real-hooks-findings.md, ranked by severity. Big-picture observation: writing the Trusted Rust hook (F4) was easier than writing the declarative predicate hook (F1 + F2 + F3) — three of seven findings target predicate-authoring ergonomics. The declarative path needs the most polish before third-party extension authors will trust it for non-trivial policy. Tests: 6 new in real_hooks.rs, all pass. * feat(hooks): close all remaining threat-model and ergonomics gaps Closes the Med-priority threat-model gaps (I2, D5, poison runbook) and all real-hooks ergonomics findings (F2, F3, F5, F6, F7) in a single pass. Threat model: - I2 (resolver field-scope): documented in SanitizedArguments rustdoc. The narrow public surface (only is_resolved + extract_numeric) enforces field-scope by construction for the current predicate path. Reassess when Installed-WASM lands. - D5 (evaluator state ceiling): MAX_HISTORY_KEYS = 8192 per map, LRU eviction with evictions_observed() metric for operator monitoring. New regression test lru_eviction_increments_counter_and_drops_oldest_key. - Poison-stickiness runbook: new docs/operator-runbook.md with recovery options ranked by cost. Ergonomics findings: - F2 (closed-vocab deny reasons): rustdoc on OnExceededAction and GateDecisionView::Deny explaining the audit-vs-model split and why manifest reason text doesn't reach the model. - F3 (NumericSum can't be TDD'd outside Reborn): new test-support feature flag with SanitizedArguments::for_tests(value) that external hook authors can opt into via dev-dep. - F5 (two ExtensionId types): added From<&ironclaw_host_api::ExtensionId> impl for identity::ExtensionId, plus cross-link rustdoc. - F6 (HookManifestEntry struct-literal fragility): added #[non_exhaustive] + HookManifestEntry::new(id, kind, body) + with_scope/with_phase/with_priority/with_description/with_requires_grant builder methods. Migrated 3 external call sites in tests/. - F7 (priority guidance): rustdoc on HookPriority with when-to- deviate guidance, named FIRST/LAST constants documented for Builtin/Telemetry use cases. Tests: 151 unit + 1 + 6 integration in ironclaw_hooks all pass with --all-features. ironclaw_reborn (13 hooks_integration scenarios) unchanged. Threat model updated: I2 / D5 / poison runbook marked closed in both the per-vector table and the cross-cutting properties matrix. Open follow-ups now down to two Low items (I4 timing side-channel residual, I5 instruction-marker denylist refresh) plus the deferred DenyReasonCode enum from F2. * fix(ci): collapse nested match in hooks_integration test for clippy --all-features CI runs `cargo clippy --all --tests --examples --all-features -- -D warnings` which is stricter than the workspace clippy I ran locally and trips `clippy::collapsible_match` on the nested-if in HookDecisionEmitted matching. Collapse the inner `if decision.kind_name() == "deny"` into an arm guard. * feat(hooks): address henrypark133 review — Critical #1/#2/#3/#5, Concerning #5/#7 Address composition-seam bugs in the Reborn factory wiring + doc tidy. henrypark133 review findings addressed: Critical #1 — before_prompt hook messages not materialized. HookedLoopPromptPort now requires a HookPromptMaterializationSink and fails closed if patches are emitted without one. The reborn factory installs an InstructionStoreBackedHookSink adapter that delegates to the host's InstructionMaterializationStore, so synthetic msg:hook.* refs are resolvable by the downstream model resolver. New seam trait (HookPromptMaterializationSink) keeps ironclaw_hooks decoupled from LoopRunContext. Critical #2 — OwnCapabilities hooks were inert in production wiring. Factory now installs SurfaceBackedProviderResolver (consults the visible-capability surface for capability_id → provider). With this, ctx.provider is populated and OwnCapabilities-scoped Installed hooks actually fire against their own provider's capabilities. Critical #3 — gate refs were unresolvable. Middleware default switched from UuidHookGateRefFactory to FailClosedHookGateRefFactory. Tests must explicitly opt into UUID (via with_gate_ref_factory) to exercise the affirmative ApprovalRequired path; production deployments must install a router-backed factory. New factory method RebornLoopDriverHostFactory::with_hook_gate_ref_factory. Concerning #5 — AfterModel fired twice + before durable finalization. Removed AfterModel dispatch from HookedLoopModelPort; the transcript port's finalize_assistant_message is now the sole AfterModel boundary (the durable one). Model port wrapper is preserved as a no-op shim for symmetry + future model-response-observed point. Concerning #7 — doc tidy: - CLAUDE.md: 3 trust classes → 4 (Builtin/Trusted/Installed/SelfAuthored with explicit note that SelfAuthored is run-scoped only and not loadable from an external source). - operator-runbook.md: "Audit log" → "durable runtime event stream" where the projection is actually the runtime-event stream, not formal AuditEnvelope records. - prior-art.md: poison-lifetime nuance — per-host-build with the factory pattern, process-lifetime only for the legacy adapter. - prior-art.md:80: trailing whitespace removed. Testing gaps from henrypark133 — caller-level tests through RebornLoopDriverHostFactory: #1 (before_prompt resolver path): before_prompt_hook_message_is_resolvable_via_factory_wiring #2 (OwnCapabilities positive/negative/unknown): own_capabilities_hook_fires_when_provider_matches own_capabilities_hook_does_not_fire_when_provider_differs own_capabilities_hook_does_not_fire_when_provider_unknown #3 (pause/auth gate lifecycle or fail-closed): pause_approval_with_default_factory_fails_closed_as_denied pause_approval_hook_surfaces_as_approval_required_with_real_gate_ref (updated to require explicit UuidHookGateRefFactory opt-in) #5 (AfterModel exactly-once at durable boundary): after_model_fires_exactly_once_at_durable_boundary Still TODO from review (separate commits): Critical #4 (telemetry context — two-run attribution) + gap #4 Concerning #6 (TimelineEntry hook metadata projection) + gap #6 Tests: 154 unit + 18 hooks_integration + all other reborn tests pass. Workspace clippy + fmt + no-panics clean. * feat(hooks): address remaining henrypark133 review — Critical #4, Concerning #6 Critical #4 — per-run hook telemetry attribution. New `HookDispatcherBuilderFactory` signature: factory returns a HookDispatcherBuilder, and `RebornLoopDriverHostFactory` attaches a `RunScopedHookMilestoneSink` keyed to the CURRENT run's LoopRunContext inside `build_text_only_host_with_capabilities`, before sealing the dispatcher. The previous zero-arg signature relied on the closure capturing run_context — silently misattributed across reuses; new public API `with_hook_dispatcher_builder_factory` removes that failure mode entirely. Legacy `with_hook_dispatcher_factory` retained for back-compat (its sink-wiring contract stays caller-side). Concerning #6 — TimelineEntry hook metadata. Added 6 optional fields to `TimelineEntry` (hook_id, hook_point, hook_trust_class, hook_decision, hook_failure_category, hook_failure_disposition) and projected them from `RuntimeEvent::Hook*`. Replay consumers now see which hook fired/failed, not just that some hook event happened. Each field is closed-vocabulary (no free-form reason text — that stays in the audit reason payload, not the product replay DTO). Testing gaps from henrypark133 — caller-level tests: #4 (two-run hook telemetry attribution): hook_telemetry_attribution_is_per_run_not_captured Builds two hosts from the SAME builder factory closure with two fresh LoopRunContexts. Asserts each run's hook milestones carry its OWN run_id (no stale captured one). #6 (replay projection contract for hook events): hook_runtime_events_project_with_sanitized_hook_metadata non_hook_runtime_events_project_with_no_hook_metadata Constructs RuntimeEvent::Hook{Dispatched,DecisionEmitted,Failed} and asserts the projection preserves the metadata fields. The negative test guards against cross-contamination on non-hook events. All henrypark133 review items now addressed: Critical: #1, #2, #3, #4 — done Concerning: #5, #6, #7 — done Testing gaps: #1-#6 — done Tests: 154 unit + 19 hooks_integration in ironclaw_reborn + 61 reborn unit + 38 + 2 new in ironclaw_event_projections + ... pass. Workspace clippy + fmt + no-panics clean. * docs(hooks): scope DenyReasonCode closed-vocabulary enum (successor #6) Successor PR from #3573 — real-hooks ergonomics finding F2 (deferred). Adds a curated vocabulary of model-visible denial reasons so hook authors can communicate why a deny happened without opening a free-form prompt-injection channel. * feat(hooks): DenyReasonCode + PauseReasonCode closed-vocabulary enums Address real-hooks ergonomics finding F2 (deferred from PR #3573). The prior dispatcher collapsed every Installed-tier deny to the static label 'hook_predicate_denied', because manifest reason strings are author-controlled and surfacing them to the model would open a prompt-injection channel. The cost: the agent couldn't tell *why* a hook denied. This PR introduces two closed-vocabulary enums: - DenyReasonCode: Generic / RateLimit / ValueCap / Blocklist / RequiresApproval / OutOfPolicy - PauseReasonCode: Generic / RequiresApproval / OverThreshold / SensitiveAction Each variant has an as_label() returning &'static str (so the sink's &'static str contract is preserved). New OnExceededAction variants 'DenyWithCode { code, reason }' and 'PauseApprovalWithCode { code, reason }' let manifest authors opt into the richer labels while keeping reason audit-only. The legacy Deny { reason } / PauseApproval { reason } variants are retained for back-compat and map to DenyReasonCode::Generic / PauseReasonCode::Generic — existing manifests continue to produce hook_predicate_denied / hook_predicate_pause_requested. Threat-model regression: a hook author cannot smuggle text into the model-visible label because the 'code' field is typed as the enum; there's no String slot exposed model-side. A test (deny_with_code_only_exposes_enum_variants_to_model) documents this as a compile-time property. Tests (+7 new = 161 total): - deny_reason_code_labels_are_stable: pins the label vocabulary so rename/relabel is loud. - pause_reason_code_labels_are_stable: same for PauseReasonCode. - deny_with_code_round_trips_through_json + pause variant: wire round-trip + snake_case tag assertion. - deny_with_code_only_exposes_enum_variants_to_model: compile-time property check. - rate_or_value_cap_with_deny_code_routes_to_code_label: end-to-end affirmative test that the dispatcher emits the code's label. - rate_or_value_cap_with_pause_code_routes_to_code_label: same for pause. Scope doc: crates/ironclaw_hooks/docs/successors/06-deny-reason-code.md * test(hooks): address codex review on #3636 - Update stale real-hooks-findings.md F2 row to cite this PR's enum follow-on (was 'deferred'). - Add install_deny_with_code_manifest_surfaces_code_label_on_dispatch: end-to-end test driving the registrar->dispatcher path for the new DenyWithCode variant (prior tests covered serde + direct hook evaluation, but not the manifest install path that downstream authors actually use). Codex review on PR #3636: APPROVE with two recommendations; both addressed. Tests: 162 unit (+1 new). Clippy/fmt clean. * fix(hooks): attenuate Installed-tier prompt patches to user role Installed-tier `before_prompt` patches were injected as role:"system" messages. Envelope text labels ("[ext-foo says]: ...") do not strip system-role authority from the model's perspective, so a third-party extension could inject system-tier instructions through a snippet patch. This is a prompt-authority escalation against the trust hierarchy the framework otherwise enforces. Add `role_for_trust_class()` mapping Installed -> "user" and Builtin/Trusted/SelfAuthored -> "system". Thread per-patch trust_class through `wrap_patches_to_messages` and use it for the emitted `LoopModelMessage.role`. Tests: - installed_hook_patch_drops_to_user_role: asserts the role for an Installed-tier patch is "user" - trusted_tier_hook_patch_keeps_system_role: regression that Trusted tier still produces system-role content * fix(hooks): enforce scope filter on observer dispatch + reject incompatible points Two related defense-in-depth fixes against silent scope-filter failure: 1. The registry silently accepted Installed bindings with `HookBindingScope::OwnCapabilities` at points (BeforePrompt, AfterModel, AfterCheckpoint) whose dispatch context carries no per-capability provider. The manifest's declared scope had no effect at all — the hook fired against every dispatch. Reject the binding at install time so the operator sees the misconfiguration. 2. `dispatch_observer_at` for `AfterCapability` did not consult the binding's scope, so an Installed observer registered with `OwnCapabilities` fired against every invocation regardless of provider. Add `dispatch_observer_at_with_provider` carrying the resolved capability provider; the capability-port middleware resolves the provider once per invocation and threads it through both the BeforeCapability hook context and the AfterCapability observer dispatch. The dispatcher then enforces `HookBindingScope::permits` on each observer binding. `ObserverHookContext` gains a `provider: Option<ExtensionId>` field; `#[non_exhaustive]` keeps existing authors compiling. Tests: - rejects_own_capabilities_at_before_prompt - rejects_own_capabilities_at_after_model - accepts_own_capabilities_at_before_capability - own_capabilities_observer_filters_foreign_providers (covers foreign / matching / unresolved provider) * fix(hooks): preserve free-form audit reason alongside closed-vocab model label (serrrfirat #3636) `PredicateBackedBeforeCapabilityHook::evaluate()` was discarding the free-form `reason` from `EvaluatorDecision::{Deny, PauseApproval}` with `..` and only sending `code.as_label()` into the sink. The `HookDecisionEmitted` milestone therefore carried only the closed- vocab label, and operator-visible audit/SSE context was silently lost end-to-end. The fix splits the channels: - Model sees the closed-vocab label (`hook_rate_limit`, `hook_pause_over_threshold`, ...) via `sink.deny(label)`. This channel is unchanged. - Audit/SSE sees the manifest's free-form `reason` via a new audit-only sink method `record_audit_reason(reason: String)`. The recording sink captures it; the dispatcher reads it after the hook returns and threads it into `LoopHostMilestoneKind::HookDecisionEmitted`. Surface changes: - `PrivilegedGateSink` / `RestrictedGateSink` gain `record_audit_reason(String)` — accepts dynamic `String` (audit-only, no model-facing seam) unlike the `&'static str` decision reasons. - `RecordingGateSink` gains an `audit_reason: Option<String>` field. - `GateHookOutcome::Decision` is now `Decision { decision, audit_reason }`. - `HookDispatcher::emit_decision_with_audit` threads the audit reason into the milestone. - `LoopHostMilestoneKind::HookDecisionEmitted` gains a `#[serde(default, skip_serializing_if = "Option::is_none")]` `audit_reason: Option<String>`. The durable RuntimeEvent projection intentionally drops this field — audit reasons are operator-facing in-memory SSE content, never durable cross-process surface. Tests: - `deny_with_code_records_audit_reason_separately_from_model_label`: asserts the recording sink ends with `Deny { reason: "hook_rate_limit" }` in `state` AND `audit_reason == Some("daily cap of $1000 ...")`. * fix(hooks): remove unused model_request helper (CI clippy fix) * fix(hooks): address serrrfirat P1/P2 findings on PR #3573 Three issues from the 5-15 review: **P1 #1 registrar.rs:70 — `same_tenant` grants not enforced** `HookManifestEntry::validate` only confirmed `requires_grant` was present; the registrar then immediately installed the binding with no host-verified grant context. A manifest could declare `requires_grant = "anything"` and get a cross-extension binding for free. Fix: `HookRegistrar` now carries a `verified_grants: HashSet<String>` (empty by default — default-deny). Add the host-facing setter `with_verified_grants(...)`. At `install_one`, if `entry.requires_grant` is `Some(g)`, require `g ∈ verified_grants` or reject with a clear error. Tests: - `install_rejects_same_tenant_without_verified_grant` - `install_rejects_same_tenant_when_verified_grants_mismatch` - The existing positive test `installer_propagates_owning_extension_and_scope_from_manifest` now wires the verified grant explicitly (proves the API contract). **P1 #2 prompt_port.rs:150 — zip misalignment** The materialization loop zipped surviving messages against the ORIGINAL unfiltered patch list. `wrap_patches_to_messages` skips metadata patches and over-budget snippets, so the zip silently paired message[0] with patch[0] even when patch[0] was the skipped metadata — materializing the wrong content (or none) under the snippet's synthetic ref. Fix: `wrap_patches_to_messages` now returns `Vec<WrappedHookMessage { message, safe_content }>` — surviving messages paired with their content by construction. The caller materializes `entry.safe_content` under `entry.message.content_ref` directly; no zip against unfiltered input. Removed the now-unused `safe_content_for_patch` helper. Test: - `materialization_stays_aligned_when_metadata_patches_are_filtered`: a hook emits `[metadata, snippet]`; asserts only one model message, and the materialized content under its ref contains the snippet's body — proves filtering can no longer desync from materialization. **P2 #3 loop_driver_host.rs:1343 — `with_hook_dispatcher_builder`** Docs said it deferred `build_arc()` to let the host factory finalize wiring; the implementation called `build_arc()` eagerly and routed through the legacy shared-dispatcher adapter, losing per-run dispatcher isolation and the run-scoped milestone sink. Fix: marked `#[deprecated]` with a note pointing callers to `with_hook_dispatcher_builder_factory(|| ...)` for per-build isolation, or `with_hook_dispatcher(...)` if they actually meant the shared adapter. The method body is unchanged so no callers break; they'll see the deprecation warning. No internal callers exist, so the deprecation doesn't trip `-D warnings`. All 162 hooks lib + 19 reborn integration tests pass; clippy clean. * fix(hooks): address serrrfirat 3573-2026-05-15 review findings P1 — prompt bundle authority mismatch (prompt_port.rs): `HookedLoopPromptPort::build_prompt_bundle` called the inner port first, which caused `HostManagedLoopPromptPort` to issue the prompt-bundle authority grant against the pre-hook message list. The wrapper then appended `msg:hook.*` messages to `bundle.messages`, so the downstream model request hit `grant.messages != messages` and failed closed with "model request messages do not match the host-built prompt bundle". Add `with_bundle_authority(authority, run_context)` and re-issue the grant after appending hook messages so it covers the post-hook bundle. Reborn wires `prompt_authority.clone()` + `run_context.clone()` into the wrapper at construction time. P2 — observer installer accepts non-observer points (dispatch.rs): `install_observer` accepted any `HookPointSpec` (including `BeforeCapability` / `BeforePrompt`) and only populated the observer map. Dispatch later found a binding without a gate/mutator impl and fail-closed the capability with "binding present without installed implementation". Reject non-observer points at install time so misuse fails loudly rather than poisoning bindings at dispatch. P2 — batch path skipped AfterCapability observers on inner error (capability_port.rs): The batch loop used `?` directly on `self.inner.invoke_capability(...)`, which propagated the error before dispatching `AfterCapability` observers. Failed batch entries disappeared from telemetry / audit, while the single-invocation path dispatches observers on error. Capture the inner result, dispatch observers, then propagate the error. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): address PR #3573 review feedback round 3 Addresses serrrfirat's CHANGES_REQUESTED review (2026-05-20) by tightening several install-time / dispatch-time bounds and gating production seams: - Bound free-form audit reasons crossing telemetry. New `telemetry::sanitize_audit_reason` strips control characters and caps length at 512 bytes; `emit_decision_with_audit` routes the manifest- supplied reason through it before publishing milestones. Manifest validation also rejects reasons over the same byte limit at install time so the wire-side cap is a defense-in-depth layer, not the only line. - Make hot dispatch O(H) instead of O(H^2). The per-binding poison recheck used to acquire the registry mutex and walk every binding; `ordered_bindings_with_poison_snapshot` now takes the active bindings and the poisoned hook-id set under a single lock, and each loop threads a local `HashSet<HookId>` that absorbs mid-dispatch poisoning. Removed the redundant `is_poisoned` helper. - Gate `HookDispatcher::registry_for_test` behind `cfg(any(test, feature = "test-support"))`. The accessor previously exposed `&Mutex<HookRegistry>` in production, letting any `Arc<HookDispatcher>` holder lock and call `HookRegistry::poison` to disable installed hooks. Added `active_bindings_snapshot(point)` as the read-only production-safe replacement. - `#[serde(deny_unknown_fields)]` on every hook-manifest and predicate DTO (`HookManifestEntry`, `HookManifestBody`, `WasmBudget`, `HookPredicateSpec`, `CapabilityPredicate`, `ValueOrRateBound`, `OnExceededAction`). Typoed or unsupported fields (e.g. a manifest-supplied `trust_class`) now fail loud at install time instead of being silently dropped. - Bound predicate trees at install. New `validate_predicate_tree` enforces `MAX_PREDICATE_DEPTH = 8`, `MAX_PREDICATE_NODES = 64`, `MAX_PREDICATE_STRING_BYTES = 256`, and `MAX_MANIFEST_REASON_BYTES = 512`. A hostile registry manifest can no longer install a deep or huge `All`/`Any` tree that the evaluator would recursively walk on every match. - Cap sliding-window samples per key. `MAX_SAMPLES_PER_KEY = 4_096` in the predicate evaluator. Both the invocation-count and numeric-sum histories drop the oldest sample once the cap is reached, bounding memory under attacker-triggered hot capabilities while preserving rate/value-cap semantics over the most recent window. - `split_indexer` / `resolve_path` now fail closed on malformed bracket syntax (`amount[foo]`, `amount[`, trailing garbage). Previously they silently fell back to the parent field, which could let a typoed `NumericSum` predicate evaluate against the wrong value and allow calls the predicate would otherwise have denied. - Honor `PatchOrdinalHint`. `WrappedHookMessage` carries the source patch's `ordinal_hint`; `HookedLoopPromptPort` inserts `NearTop` messages after the bundle's `identity_message_count` and appends `Last` messages at the end. Safety/policy snippets that need early placement now get it. - Update `ironclaw_hooks` top-level docs to reflect the four trust classes (`Builtin`/`Trusted`/`Installed`/`SelfAuthored`) and the now-wired Reborn middleware composition. Tests added: - `manifest::rejects_unknown_top_level_field` - `manifest::rejects_unknown_wasm_budget_field` - `manifest::rejects_predicate_tree_exceeding_max_depth` - `manifest::rejects_predicate_tree_exceeding_max_nodes` - `manifest::rejects_predicate_string_exceeding_max_bytes` - `manifest::rejects_manifest_reason_exceeding_max_bytes` - `points::capability::malformed_indexer_returns_none_not_parent_value` - `telemetry::sanitize_audit_reason_*` (truncate / strip control / preserve / empty) `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features`, and `cargo test -p ironclaw_hooks` all pass clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(hooks): batch deferred test coverage from #3573 review (#3914) * perf(hooks): defer capability input resolution until a predicate needs it (#3913) * fix(rebase): adapt hooks tests + middleware to upstream API additions - CapabilityDescriptorView: add parameters_schema field - LoopModelRequest / LoopPromptBundleRequest: add capability_view field - TimelineEntry test builder: add hook_id / hook_point / hook_trust_class / hook_decision / hook_failure_category / hook_failure_disposition fields - ironclaw_reborn::tests::hooks_integration: switch from InMemoryLoopCheckpointStore to InMemoryTurnStateStore (which now impls both LoopCheckpointStore and TurnStateStore), pass TurnActor in TurnRunState, supply the new turn_state_store factory arg - ironclaw_reborn lib.rs: drop the pub-use re-exports that upstream intentionally removed (per the module-directory rationale in the current ironclaw_reborn lib.rs doc comment); update the hooks_integration test imports to use module paths - Cargo.toml: union the hooks-foundation member list with upstream's new crates (event_streams, auth, first_party_extensions, reborn_webui_ingress, product_workflow_storage, webui_v2); drop ironclaw_storage which no longer exists upstream - crates/ironclaw_architecture/tests/reborn_dependency_boundaries: keep upstream's removal of ironclaw_filesystem from the ironclaw_turns forbidden list AND add ironclaw_hooks to that list - crates/ironclaw_reborn/src/milestone_events.rs: drop dead loop_failure_kind helper (replaced upstream by loop_failure_kind_name in text_loop_driver.rs); keep hook_decision_label which is still used Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * perf(hooks): restore batched capability dispatch when hooks acti…
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…earai#3633) * docs(hooks): scope production gate-ref factory (successor #1) Successor PR scope doc. Until this lands, hook PauseApproval/PauseAuth decisions surface as Denied in production because the middleware default is FailClosedHookGateRefFactory (PR nearai#3573 / henrypark133 Critical #3). This PR carries the scope doc only; implementation follows after review of the design (cross-crate seam to approval gateway is the load-bearing decision). * docs(hooks): incorporate codex review on nearai#3633 scope Adds two addenda from codex's design-review pass: - Critical: actor/session binding requirement (prevents same-tenant wrong-user approval-bypass). Gateway reservation must carry the actor/session id and reject cross-actor consumption. - Recommendation: include capability id + arguments digest in the reservation, not just free-form reason. Lets the approval UI show the exact gated call AND defeats a future-call digest-mismatch replay vector. * Implement router-backed hook gate refs Cites Codex review addenda: bind hook gate reservations to actor/session identity and carry capability plus arguments digest before handing refs to the approval/auth router. * Fix router-backed hook gate context and TTL * fix(hooks): make gate-ref resolution time router-owned (serrrfirat HIGH nearai#3633) serrrfirat HIGH on PR nearai#3633: `HookGateResolutionRequest.resolved_at` was a caller-controllable timestamp. Any adapter wiring the request from external input — or a buggy router that trusted it — could backdate it and consume an expired approval/auth gate ref. The same caller-supplied timestamp was persisted as the reservation's `consumed_at`, so forged values also corrupted the one-shot audit trail. The `InMemoryHookGateRouter` reference implementation already reads its own wall clock (`Utc::now()`) inside `resolve_gate` for both expiry checks and `consumed_at` — but the public request struct still exposed `resolved_at` as a `pub` field, encouraging future router impls to trust it and leaving the field as ambient trust- boundary surface. Fix: remove `resolved_at` from `HookGateResolutionRequest` entirely. Time authority for resolution and consumption lives exclusively on the router's wall clock; the only place a resolution timestamp surfaces is `HookGateResolution::resolved_at` (the *result*), which is router-supplied. The `for_kind` / `for_invocation` constructors no longer take or set a timestamp. Tests: - `router_backed_pause_approval_gate_ref_rejects_backdated_resolution_after_ttl` reframed: it no longer mutates `request.resolved_at` (the field is gone). Instead it relies on the router's own clock — TTL = 1ms, sleep 5ms, resolve must surface `Expired`. The property is now statically enforced by the absence of the field rather than dynamically asserted, but the regression test still exercises the router-owned-time code path. * fix(hooks): address henrypark133 must-fix #1, #2, #3, #5 on PR nearai#3633 Four items from the 5-15 review: **#1 (must-fix) MAX_RESERVATION_TTL cap** `RouterBackedHookGateRefFactory::try_new` now caps `reservation_ttl` at 24h. Without the cap, an operator misconfiguring a year-long TTL accumulates unresolved reservations in `InMemoryHookGateRouter` state for the full window — a long-tail memory leak. 24h is plenty for human-in-the-loop approval flows. **#2 (must-fix) MAX_REASON_BYTES cap** `mint(...)` rejects `reason` strings longer than 4 KiB. Without the cap, a buggy or malicious caller could push arbitrarily large strings through the approval store; the reason is operator-facing and may be persisted. **#3 (must-fix) Split `InvalidDigest` from `InvalidToken`** `validate_token` previously returned `HookGateError::InvalidDigest` for failures on actor / session ids — confusing because those values aren't digests. Add a new `InvalidToken { field, reason }` variant and route `validate_token` to it. `InvalidDigest` stays for actual sha256-digest shape failures. **#5 (must-fix) Collapse consumption-failure oracle** `From<HookGateError> for AgentLoopHostError` previously preserved Display text for every variant, so a probing caller could distinguish "this gate ref doesn't exist" from "this gate ref belongs to another run/actor/capability" — an oracle for liveness detection on foreign gate refs. Now collapses the entire consumption-failure family (`UnknownGate` / `AlreadyConsumed` / `Expired` / `KindMismatch` / `RunMismatch` / `ActorMismatch` / `CapabilityMismatch` / `ArgumentsDigestMismatch`) to a single opaque "hook gate consumption denied" surface. Misuse / availability variants still surface details — they signal config bugs and need operator visibility. Internal variants stay distinct for test assertions and operator-visible tracing. **Bonus** (henrypark133 non-blocking #9): Add a `tracing::warn!` at the conversion site so operators can still distinguish security rejections from availability failures in logs even though the public `AgentLoopHostError` no longer carries that information. All 23 hooks_integration tests + 43 reborn lib tests still pass. * fix(hooks): host-owned per-build hook-gate factory builder (serrrfirat MEDIUM on PR nearai#3633) `RouterBackedHookGateRefFactory::try_new` takes a caller-supplied `Fn() -> HookGateReservationContext` closure, and the host factory's `with_hook_gate_ref_factory(Arc<dyn ...>)` stored ONE factory instance that was reused for every host build. That instance carried whatever run/actor context its closure captured at construction time — so a second host build could mint a gate ref against the FIRST build's `LoopRunContext`. The verification test that backdated `resolved_at` proved the router rejects stale timestamps, but didn't address the host-side wiring footgun. Added per-build callback path: - New `HookGateRefFactoryBuilder` type alias for `Arc<dyn Fn(&LoopRunContext) -> Arc<dyn HookGateRefFactory>>`. - `with_hook_gate_ref_factory_builder(F)` on `RebornLoopDriverHostFactory` installs the callback. It runs once per `build_text_only_host*` call with the active `LoopRunContext`, so production callers wire `move |run_ctx| Arc::new(RouterBackedHookGateRefFactory::try_new(..., ttl, || HookGateReservationContext::new(run_ctx.clone(), actor.clone()))?)` and the factory is constructed fresh per host with no stale capture. - Build path consults the builder first, falls back to the shared `hook_gate_ref_factory` if only the older API is wired. - `with_hook_gate_ref_factory(Arc<dyn ...>)` is marked `#[deprecated]` pointing to the builder. The method body is unchanged for back-compat. Tests: - Existing integration tests migrated to the builder API (`with_hook_gate_ref_factory_builder({ let f = Arc::new(factory); move |_| Arc::clone(&f) })`) so they exercise the same logical wiring against the new seam. All 23 pass; clippy clean with `-D warnings`. The trait/router types and `validate_token` route are unchanged from the previous fix in this PR.
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…nearai#3912/nearai#3913 (nearai#3921) * refactor(hooks): align ExtensionId/HookLocalId with newtype template Address henrypark133 approval-with-followup items from PR nearai#3912: - L1: remove dead empty-id guard in HookManifestEntry::validate. HookLocalId::new now rejects empty strings at construction, so manifest deserialization fails before validate() is ever called. - L2: add AsRef<str>, From<Self> for String, and into_inner() to ExtensionId and HookLocalId per the canonical newtype template in .claude/rules/types.md. into_string() is retained as a thin alias for compatibility; new code should prefer into_inner(). - L3: document why the builtin_id_distinct_from_extension_id test fixture substitutes "path.module" for the original "path::module" (the new grammar rejects colons in HookLocalId). No behavioral change. * fix(hooks): fire AfterCapability observer for hook-suspended entries after allowed entries henrypark133 M1 on PR nearai#3911: in `HookedLoopCapabilityPort::invoke_capability_batch`, when Phase 1 produced a mix of Pending (hook-allowed) and Resolved (hook-suspension) slots with the suspension appearing AFTER an allowed entry and `stop_on_first_suspension = true`, the merge loop initialized `stopped_on_suspension` from `stopped_in_preflight` and broke after the very first iteration. Result: trailing Resolved suspension slots never fired their `AfterCapability` observer and never surfaced in the merged `outcomes` vec, violating the per-entry observer contract from PR nearai#3573 (serrrfirat P2 #3). Fix: continue iterating the merge loop so every slot fires its observer and every Resolved outcome is pushed. Only Pending slots are dropped after a stop (their inner work was already short-circuited in Phase 1 or by an early inner-port stop), tracked via `pending_after_stop`. Re-pop guard on `inner_outcomes.pop()` keeps the previous "inner stopped early on its own suspension" semantics: pending slots without an inner outcome are dropped, but the loop continues so any trailing Resolved observers still fire. New regression test `batch_invocation_fires_observer_for_hook_suspended_entry_after_allowed_entry_with_stop_on_first_suspension` pins the behavior: `[alpha=hook-allowed, beta=hook-suspension]` with `stop_on_first_suspension = true` produces a 2-entry `outcomes` vec (Completed alpha + ApprovalRequired beta), fires the observer twice, and only sends alpha to the inner port. Verified TDD-style: the test fails on the pre-fix code with `outcomes.len() = 1`. * perf(hooks): reuse serialized argument bytes in lazy resolve henrypark133 L1 on PR nearai#3913: `resolve_arguments` measured the post-resolver JSON payload by calling `serde_json::to_vec(&value)` and discarding the `Vec<u8>` once its length was checked. The `SanitizedArguments::from_json` constructor on the happy path sanitizes the in-memory `serde_json::Value` directly without re-serializing, so the materialized buffer was pure overhead. Switch the size measurement to `serialized_len`, a counting `io::Write` adapter that streams `serde_json::to_writer` into a u64 counter — saves one Vec<u8> allocation and the matching drop per resolved invocation. Behavior is identical: the same JSON encoding rules drive both writers, the cap check still fires when the encoded length exceeds `MAX_PREDICATE_INPUT_BYTES`, and serialization errors still fail closed. No new tests required; existing `dispatch_fails_closed_when_input_exceeds_max_bytes` and the ordering / lazy-probe tests exercise this path. * test(hooks): cover mixed needs_input hook binding short-circuit henrypark133 L2 on PR nearai#3913: add `before_capability_needs_input_returns_true_when_any_active_binding_needs_input`. Installs two BeforeCapability bindings on the same scope (Global) — one `needs_input() = false`, one `needs_input() = true` — and asserts both ends of the short-circuit: 1. `HookDispatcher::before_capability_needs_input(None)` returns true. 2. Driving `HookedLoopCapabilityPort::invoke_capability` with an instrumented `ProbingResolver` confirms the resolver IS consulted exactly once — i.e. the short-circuit fires through the call site, not just the helper. This pins the "any input-needing binding wins" contract end-to-end so a future change to the dispatcher's probe (or to the middleware's lazy-probe gate) can't silently regress to short-circuiting on the first binding only. Also tightens the merge-loop's pending-slot drop path to use `Option::map` (clippy::manual_map) — cosmetic, no behavior change.
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…3920) * Implement installed WASM hook runtime Adds crates/ironclaw_hooks/docs/threat-model-wasm.md and follows the reviewed design ack: 1) module bytes are resolved, digest-cached, and compiled in the tool-WASM style while reusing its resource limiter; 2) each invocation gets a fresh wasmtime Store; 3) the ABI is a wasmtime::Linker surface, not wit-bindgen; 4) host-import sink shims enforce call, patch-byte, observer-fact, and decision budgets. * Harden WASM hook string and metadata budgets * fix(hooks): validate WASM hook ABI at install time (serrrfirat #3 on PR nearai#3634) Address serrrfirat MEDIUM finding #3: `WasmHookRuntime::prepare()` compiled and cached module bytes but did not validate imports or the requested export. ABI mismatches (unsupported import, missing export, wrong export signature) were deferred to first live dispatch — and the prior `wasm_unsupported_host_import_fails_closed` test codified that a bad-import module would install successfully and only fail closed at invocation. Malformed untrusted modules should never reach live traffic. Changes: - `prepare()` derives the target hook point from `request.kind`, then runs `validate_module_abi()`: scratch-instantiate the module against the point-specific linker (catches unsupported / wrong-type imports) and resolve the typed export `() -> ()` (catches missing export and wrong signature). Failures surface as new `WasmHookRuntimeError::InvalidImports` or existing `WasmHookRuntimeError::InvalidExport`, both of which bubble up as `HookError::RegistryConstruction` from the registrar. - `wasm_point_for_kind(HookManifestKind)` helper centralizes the kind → wasm-point mapping; the previous `execute_*` paths can share it in a follow-up but kept inline for now to minimize churn. Tests: - `wasm_unsupported_host_import_is_rejected_at_install_time`: replaces the prior test that codified late-failure behavior; asserts the registrar returns `RegistryConstruction` citing the bad import. - `wasm_missing_export_is_rejected_at_install_time`: new module that compiles but lacks the manifest-declared export; same install-time rejection. * fix(hooks): address henrypark133 must-fix #1, #2, #3 on PR nearai#3634 Three items from the 5-15 review: **#1 (must-fix) Extract ironclaw_wasm_limiter micro-crate** Replace `#[path = "../../../ironclaw_wasm/src/limiter.rs"]` cross-crate file import with a proper Cargo edge. The 111-line `WasmResourceLimiter` moves into a new `crates/ironclaw_wasm_limiter` micro-crate that both `ironclaw_wasm` and `ironclaw_hooks` depend on. The architecture rule forbidding `ironclaw_hooks -> ironclaw_wasm` is preserved (the new crate sits below both consumers and pulls in only `wasmtime` + `tracing`); `cargo check`, `cargo doc`, and architecture-linting tests now see the edge, and the file can't be moved out from under one of the consumers silently. Mechanical changes: - new `crates/ironclaw_wasm_limiter/` (Cargo.toml + src/lib.rs with the type exposed as `pub` instead of `pub(crate)`) - workspace `members` entry added - `crates/ironclaw_wasm/src/limiter.rs` deleted - `crates/ironclaw_wasm/src/lib.rs`: `mod limiter` removed - `crates/ironclaw_wasm/src/store.rs`: import switched to `ironclaw_wasm_limiter::WasmResourceLimiter` - `crates/ironclaw_wasm/Cargo.toml`: dep added - `crates/ironclaw_hooks/Cargo.toml`: dep added - `crates/ironclaw_hooks/src/wasm/runtime.rs`: `#[path = ...]` block removed; import switched to the crate **#2 + #3 (must-fix) Dead WASM arms in dispatch** `run_before_capability_hook`, `run_before_prompt_hook`, and `run_observer_hook` each had an early-return guard that dispatched WASM hooks with `catch_unwind` + timeout, then ALSO had a matching WASM arm in the inner `match` that ran without those protections. The prompt-path arm additionally swallowed `WasmHookFailure` via `|_| ()`, making the must-fix #2 problem worse on that path specifically. If a future refactor removed any of the early-return guards, those inner arms would silently take over and drop panic isolation, deadline enforcement, AND (for prompts) the failure category. Replaced each inner arm with `unreachable!()` carrying a comment that explains why the arm exists and references the early-return guard above it. A future refactor that removes the guard will now trip the `unreachable!` at first call instead of silently degrading. All 154 hooks lib + 29 reborn integration tests still pass. * fix(hooks): plumb context to WASM hooks + runtime hardening Critical #1 on PR nearai#3634: WASM hooks previously received no context. The `execute_*` entry points dropped the `&BeforeCapabilityHookContext` / `&BeforePromptHookContext` / `&ObserverHookContext` value and invoked the guest export with `()`, so a WASM gate could never decide based on the capability name, tenant, provider, or other dispatch-time facts. Add an `ic:hooks/context@1` host-import module exposing two read-only calls — `ctx_size() -> i32` and `ctx_read(ptr, len) -> i32` — backed by a JSON-serialized blob the dispatcher writes per-invocation into the fresh store. Modules that don't import these continue to link; modules that do import them get a stable, non-empty payload to read. An integration test (`wasm_before_capability_hook_reads_context_blob`) asserts the contract end-to-end: a guest that fails to read a non-empty blob traps before its `deny` call. Also rolls up the other reviewer-flagged WASM runtime issues, all of which touch `wasm/runtime.rs`: HIGH #2: epoch-tick background thread now holds a shutdown `AtomicBool` and joins on `Drop`. Previously it looped forever and leaked an Engine clone on every runtime drop. MED #4: compiled-module cache is now an `lru::LruCache` bounded by `MODULE_CACHE_CAPACITY = 128`. Replaces the unbounded `HashMap`. MED #7: `prepare()` no longer compiles under the cache lock. Fast path reads from LRU under a brief lock; slow path compiles outside the lock and re-checks on insert to avoid the TOCTOU window where two concurrent installs of the same module both compile. Bug #9: post-call `deadline_exceeded()` re-check on the Ok branch is gone. wasmtime epoch-interrupt is the authoritative wall-clock signal; an Ok return is no longer reclassified as a timeout because the wall ticked over during host-side return. Bug #10: `add_milestone_metadata` returns a distinct "metadata value exceeds the u32 byte-length ceiling" error when the guest-supplied `value.len()` overflows u32, instead of misreporting it as "exceeded total prompt-patch byte budget". Existing integration tests for WASM hooks are also re-wired through `HookRegistrar::with_verified_grants` so the grants-store gate added in the foundation-01 merge stops failing the pre-existing fixtures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): run WASM hooks on the blocking pool HIGH #3 on PR nearai#3634: `tokio::time::timeout` does NOT cancel synchronous wasmtime execution. The previous code awaited a `catch_unwind(async { h.evaluate(ctx) })` future whose body completed in one poll, so the timeout could only fire *around* the WASM call rather than against it; a hook that wedged inside wasmtime simply pinned the calling tokio task. Route gate, prompt, and observer WASM dispatch paths through `tokio::task::spawn_blocking` via a shared `run_wasm_blocking` helper. The outer `tokio::time::timeout` now governs the JoinHandle, so a stuck blocking task stops blocking the dispatcher's caller; the wasmtime epoch interrupt configured in the runtime (10 ms tick) is the authoritative in-WASM wall-clock cancel signal. JoinError (panic in the blocking task) maps to `FailureCategory::Panic`, matching the pre-existing semantics for synchronous panics caught via `catch_unwind`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * perf(hooks): O(1) hook-id lookup via side index Finding #8 on PR nearai#3634: `set_priority`, `poison`, `is_poisoned`, and `contains_hook` all did full-registry scans over every binding at every point. Each is called per-dispatch (poison-checks on the snapshot loop in particular), so the cost is `O(registered_hooks)` per `(installed_hook, registered_hook)` pair. Maintain a denormalized `HashMap<HookId, (HookPointSpec, usize)>` side index in lock-step with `by_point` so every per-hook-id operation becomes a single hash lookup + a direct vec indexed access. The duplicate-id rejection in `insert` now reads from the side index too, turning what used to be a flat-map scan into a `HashMap::contains_key`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(hooks): wall-clock timeout, observer memory, limiter rollback, registrar happy path Round out the test set for the WASM hook execution path: #11 / #12: gate + observer wall-clock timeout. The pre-fix dispatcher ran wasmtime synchronously on the executor, so the outer `tokio::time::timeout` `Err(_elapsed)` arm was effectively unreachable. Now that WASM execution runs on the blocking pool, the timeout actually fires; the new tests give the wasm budget headroom (1B fuel, 5s wall) and the dispatcher a 20 ms timeout, then assert the failure classification (FailClosed for gate, FailIsolated for observer). #13: observer memory exhaustion. Mirrors `wasm_memory_exhaustion_fails_closed_for_gate` against the observer dispatch path so the FailIsolated branch of the failure matrix has explicit memory coverage, not just fuel/wall. #15: `WasmResourceLimiter::memory_grow_failed` rollback. Stages an approved grow, simulates the OS-level grow failing, and asserts a subsequent grow of the full ceiling succeeds — the inflated `memory_used` from the failed attempt must be released. #16: registrar WASM happy path. Companion to the existing `install_wasm_body_requires_runtime` negative case: a valid module installs, the binding is visible via the public registry accessor, and is not pre-poisoned. #14 (`add_milestone_metadata` happy path) is intentionally omitted — the BeforePrompt dispatch path is currently unreachable due to a pre-existing manifest-vs-registry scope conflict (`OwnCapabilities` is the only valid `BeforePrompt` scope per manifest validation, but the registry rejects `OwnCapabilities` at `BeforePrompt` because the point has no provider context). That contradiction sits outside this PR's scope; flagging for a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(hooks): typed WASM version material, reconcile design doc LOW #20 on PR nearai#3634: extract the `{extension_version}+wasm:{module_digest_hex}` concatenation into a `WasmVersionMaterial` newtype with a single `Display` impl. The identity material no longer floats free as a stringly-typed argument inside the registrar. Reconcile `docs/successors/02-wasm-runtime.md` with the implementation: - Spell out that wall-clock cancellation depends on the `tokio::time::timeout(tokio::task::spawn_blocking(...))` pair, and explain why a bare timeout over a synchronous wasmtime call cannot actually cancel. - Define `FailIsolated` and `FailClosed` as `FailureDisposition` values, distinct from the older `HookFailureMode::{FailOpen, FailClosed}` policy switch that applies to predicates. - Clarify the generic `evaluate` export contract — name is whatever the manifest declares, signature is `(): ()`, context arrives through the new `ic:hooks/context@1` host imports — and note the intentional divergence from `WitToolRuntime`'s hardcoded interface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): drop .expect() in WASM module cache capacity Pre-commit no-panics CI flagged the .expect() on the LruCache capacity. Move the validity check to a const match, so the NonZeroUsize is fixed at compile time and the no-panics regex is satisfied. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): use HookLocalId::new after newtype privatization The newtype-privatization landed in reborn-integration after the hooks-fu-wasm-runtime branch's WASM scaffolding tests were written; update the affected test/registrar sites to use HookLocalId::new instead of the now-private tuple constructor. * style: cargo fmt after newtype-privatization fixups * test(hooks): ignore 3 BeforePrompt WASM tests with manifest/registry conflict These tests were failing on the original branch tip too (verified against origin/hooks-fu-wasm-runtime @ 571efdf). The Installed-tier BeforePrompt WASM install path has no valid scope today: - OwnCapabilities is rejected by the registry C3 check (finding #2 on PR nearai#3573) since BeforePrompt has no per-capability invocation context. - SameTenant is rejected by manifest validation ("cannot combine scope = same_tenant with kind = before_prompt"). The budget-overflow paths these tests exercise are point-agnostic; the follow-up is to either rewrite the helper to install through BeforeCapability or add a Global manifest scope. Tracked as a deferred item on the new PR. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…i#3573) (nearai#3635) * docs(hooks): scope persistent predicate counter backend (successor #3) Successor PR from nearai#3573. Current sliding-window state is in-memory and resets on restart. Adds a PredicateStateBackend trait + Postgres/libSQL impls for cross-process and restart-survival semantics. * feat(hooks): extract PredicateStateBackend trait + replay-safe in-memory impl Addresses codex review's three Critical findings on PR nearai#3635: 1. Backend wiring: the trait is now registered (lib.rs:25-26) and PredicateEvaluator delegates to Arc<dyn PredicateStateBackend> via with_backend(...). Default constructor preserves the in-memory behavior so all 154 existing tests pass unchanged. 2. Atomic record-and-read: each record_invocation / record_value call performs the write AND returns the resulting in-window count/sum under a single mutex (in-memory) / transaction (durable backends). Splitting into separate record + read would let two hosts each see 'under cap' and both proceed, drifting past max. 3. Replay refusal: each record call carries a PredicateEventId. Re-emitting the same event_id is a no-op against the count. In-memory backend implements via a per-key bounded set (RECENT_EVENT_ID_CAP = 256); durable backends will use INSERT … ON CONFLICT DO NOTHING. Trait surface (predicate_state.rs): - PredicateEventId(String): opaque dedup key - PredicateBackendError: thiserror enum for fallible durable backends; in-memory backend never returns Err - PredicateStateBackend trait with Result return types - InMemoryPredicateStateBackend default impl - MAX_HISTORY_KEYS const re-exported via evaluator for back-compat Evaluator changes (evaluator.rs): - holds Arc<dyn PredicateStateBackend> (no more inline maps) - evictions_observed() reads through to backend - synth_event_id() generates per-call-unique ids via a process-local atomic counter so tests with identical (hook, ctx, now) still produce distinct ids - LRU helpers + HistoryKey/ValueHistoryKey types moved into predicate_state.rs (as InvocationKey/ValueKey) Tests: - 6 new predicate_state tests: - in_memory_invocation_counts_within_window - in_memory_invocation_trims_outside_window - in_memory_value_sums_within_window - in_memory_tenant_isolation (regression on threat-model C2) - in_memory_duplicate_event_id_is_a_noop_for_invocations - in_memory_duplicate_event_id_is_a_noop_for_values - 160 unit tests pass total. Reborn hooks_integration unchanged at 19 scenarios. Clippy/fmt/no-panics clean. Sync trait + Instant timestamps documented as a v1 choice; durable backends (Postgres, libSQL) will need an async companion trait using SystemTime — tracked in the scope doc as the next slice. Scope doc: crates/ironclaw_hooks/docs/successors/03-persistent-counter.md * fix(hooks): close codex P1 bugs in PredicateStateBackend in-memory impl Addresses codex P1 review on PR nearai#3635: P1 #1 — replay dedup loss under high-throughput keys The prior design used a fixed-size (256) recent_ids ring per bucket decoupled from entries. Under any workload with >256 distinct events in the same window, the first event's id aged out of the ring while its timestamp entry was still live, so a replay silently re-counted. Fix: dedup memory is now intrinsic to entries. Each entry stores (timestamp, event_id), and the dedup check is 'does any in-window entry have this id?'. Dedup memory is therefore exactly the in-window entry set — no fixed cap, no silent loss. P1 #2 — zombie buckets clogging LRU Two-part fix: 1. record_* drops empty buckets eagerly via history.remove(key). This is mostly defense-in-depth — under the new dedup design, the record path can't actually leave a bucket empty (proved in the test rationale comment). 2. evict_lru_* now preferentially targets empty buckets first (find any v.entries.is_empty()), only falling back to the oldest-timestamp scan if no empty bucket exists. Filter-out behavior is gone, so any empty bucket that somehow survives becomes the next eviction victim instead of a permanent zombie. Test changes (+2 new, -0 removed): - dedup_memory_covers_full_window_under_high_throughput: pushes 512 distinct events into one bucket, then replays event-0. Pre-fix this would have counted again (silent dedup loss); post-fix the replay is a no-op. - lru_evicts_empty_buckets_first: crafts an empty bucket alongside a live one, runs LRU eviction, asserts the empty one is evicted and the live one retained. Tests: 162 unit total (+2 new). Clippy/fmt/no-panics clean. * docs(hooks): address gemini review on persistent-counter scope doc Four medium-priority doc nits from gemini-code-assist on the crates/ironclaw_hooks/docs/successors/03-persistent-counter.md scope: 1. run_id in the trait: removed. The trait dedupes on event_id (RuntimeEventId is already run-scoped), not run_id. Replaces the earlier 'backend stores (timestamp, run_id, event_id)' claim. 2. SystemTime vs chrono::DateTime<Utc>: switched to DateTime<Utc> to match project convention (src/db/mod.rs, ironclaw_events). The in-memory backend keeps Instant for monotonic process-local semantics; durable backends require DateTime<Utc> for cross- process serialization. Documented as a clock note. 3. libSQL TEXT column for rust_decimal: per src/db/CLAUDE.md, libSQL can't preserve Decimal precision with numeric/real types. LibSqlPredicateStateBackend serializes value as TEXT via Decimal::to_string() / from_str(). Postgres impl keeps numeric (correct for PG). Documented as the two LibSql-specific schema differences. 4. Batched-writes vs cross-process consistency tension: gemini was right that deferring writes to the tick boundary breaks requirement #1 (two hosts would each see 'under cap' simultaneously). v1 production backend keeps writes synchronous; future optimization batches reads (not writes). * fix(hooks): thread stable caller_event_id through hook context (replay dedup) henrypark133 HIGH on PR nearai#3635 + serrrfirat HIGH #1: the `PredicateBackedBeforeCapabilityHook -> PredicateEvaluator` path always synthesized a fresh `event_id` per evaluation by mixing in a process-local atomic counter, so the same logical invocation retried/replayed always got a different id. The backend's UNIQUE constraint on `event_id` — the load-bearing dedup contract — never engaged on the real production path. Replay dedup was effectively "documented but unused." Plumb a stable per-invocation identity through the public hook surface: - `BeforeCapabilityHookContext` gains a `caller_event_id: Option<PredicateEventId>` field. Middleware that threads through from the calling layer's runtime event identity populates `Some(...)`; older / in-memory-only callers pass `None` and degrade to the current synth path (no behavior change). - New builder method `with_caller_event_id(...)`. - `PredicateEvaluator` resolves the id through a new `resolve_event_id` helper: prefer `ctx.caller_event_id`, fall back to `synth_event_id`. Both `record_invocation` and `record_value` paths use it. - Backend dedup behavior is unchanged — it was already correct on `event_id`. The bug was the caller path never supplying a stable id. Tests (caller-boundary, henrypark133's required regression): - `duplicate_caller_event_id_is_deduped_in_invocation_count`: two evaluations with the same `caller_event_id` count as one invocation; a third with a different id counts as two; a fourth crosses the cap. Sanity branch confirms the no-id synth path still exhibits "every call counts" semantics. This is the API contract slice. Wiring the middleware to actually supply a stable id (e.g. derived from the originating `RuntimeEventId` once that runs through the BeforeCapability path) is the follow-up that lights up the durable backend's end-to-end replay-safety promise. * refactor(hooks): demote PredicateStateBackend to pub(crate) (serrrfirat MED on PR nearai#3635) serrrfirat MED: the `predicate_state` module exposed `PredicateStateBackend` as `pub`, but the trait's `now: Instant` parameter is process-local and not serializable. Any external durable backend impl built against the current trait would have to be rewritten when the durable contract lands with `chrono::DateTime<Utc>` (see successor doc 03-persistent-counter.md). Hold the public surface back until that contract is stable so we don't ship a public API we know we'll break. Demoted to `pub(crate)`: - `PredicateStateBackend` (trait) - `InvocationKey`, `ValueKey` (key types — backend ABI only) - `PredicateBackendError` (error type, with `#[allow(dead_code)]` on the `Unavailable` variant since the in-memory backend is infallible and no durable backend exists yet) - `InMemoryPredicateStateBackend` (the only impl) - `PredicateEvaluator::with_backend` (with `#[allow(dead_code)]` — reserved for future internal injection paths) Kept `pub`: - `PredicateEventId` — it appears on the public hook surface via `BeforeCapabilityHookContext::caller_event_id` (from the nearai#3635 HIGH fix). Hook authors who want stable replay-dedup ids construct one. No behavior change. All 163 hooks lib tests + 19 reborn integration tests still pass. * fix(hooks): address henrypark133 must-fix #1-5 on PR nearai#3635 Five items from the 5-15 review: **#1 (must-fix) O(n) dedup scan** The previous `bucket.entries.iter().any(...)` linear scan held the outer history mutex while walking thousands of in-window entries at high throughput. Add a companion `HashSet<PredicateEventId>` per bucket (`InvocationBucket.dedup_ids` / `ValueBucket.dedup_ids`), maintained alongside the deque via `pop_front`/`push_back` helpers. O(1) dedup, same correctness, same memory bound (one set entry per in-window entry — no fixed ring). **#2 (must-fix) Mutex poison cascade** `.expect("predicate history mutex poisoned")` propagated a panic to every subsequent caller. Replace with `match self.invocation_history.lock() { Ok(g) => g, Err(p) => p.into_inner() }` so a poisoning thread doesn't take down all subsequent evaluations. **#3 (must-fix) `caller_event_id` format validation** `with_caller_event_id` now rejects empty strings and ids containing NUL bytes. Failed validation logs a `tracing::warn!` and leaves `caller_event_id == None` so the synth path takes over — operator sees the warning, predicate dedup still works. Also: `PredicateEventId(pub String)` → `PredicateEventId(String)` with `new()` / `as_str()` (henrypark133 nit #9). Inner field is no longer in-place mutable from outside the crate. **#4 (must-fix) `with_backend` is `#[cfg(test)]`** Previously `#[allow(dead_code)]` — reachable from release builds and inviting future callers to inject backends through an unstable seam. Gated to `cfg(test)`. **#5 (important) `evict_older_than` trait stub** Default-impl no-op added to `PredicateStateBackend` so the trait signature is locked before the first durable-backend PR. Trait-object callers won't break when durable impls override it. **Bonus** (henrypark133 missing-coverage #1): `in_memory_record_invocation_is_atomic_under_concurrent_writers` — 32 threads each record a distinct event id; final count must equal 32, proving the atomic record-and-read contract holds under contention. **Bonus** (henrypark133 nit #10): The third stable id in `duplicate_caller_event_id_is_deduped_in_invocation_count` was 62 chars; bumped to 64 to match the synth output format. * fix(hooks): clippy doc-list-indentation + remove unused with_backend (nearai#3635 CI) * fix(hooks): address serrrfirat HIGH + MEDIUM on PR nearai#3635 (5-15 review) **MEDIUM — `caller_event_id` validation bypass** `with_caller_event_id` validated for empty/NUL but the field on `BeforeCapabilityHookContext` is `pub`, so callers could direct- assign `Some(PredicateEventId::new("..."))` with `new()` permissive and bypass the setter entirely. Move validation INTO the type boundary: - `PredicateEventId::new(...) -> Result<Self, PredicateEventIdError>` validates non-empty + NUL-free at construction. Any value that reaches a downstream backend now satisfies the format invariant by construction. - `PredicateEventId::new_unchecked(...)` for internal synth paths and tests that mint ids from known-good shapes (hex digests). - `with_caller_event_id` drops its now-redundant runtime check; the type already enforces it. - Internal synth in `evaluator.rs` switches to `new_unchecked` (64-char hex output is always valid by construction). Tests: - `predicate_event_id_rejects_empty` - `predicate_event_id_rejects_nul_bytes` - `predicate_event_id_accepts_typical_hex_digest` **HIGH — durable schema: dedup scope mismatch** The successor doc's Postgres schema declared `event_id uuid PRIMARY KEY` (globally unique), but the trait's replay-refusal contract dedupes within the counter `key`. `caller_event_id` is per capability invocation — two predicate-backed hooks observing the same invocation share an id. A global PK lets the first hook's INSERT win and silently undercounts the second hook's bucket. - `docs/successors/03-persistent-counter.md`: PK changes to composite `(tenant_id, hook_id, capability, event_id)` for invocations and `(tenant_id, hook_id, capability, field, event_id)` for values, matching the trait's per-key dedup scope. - `predicate_state.rs` trait doc: replay-refusal section rewritten to spell out the per-key scope and the corresponding `INSERT … ON CONFLICT (tenant, hook, capability[, field], event_id) DO NOTHING` shape durable backends should use. * docs(hooks): document host-assigned trust boundary on PredicateEventId henrypark133 / serrrfirat blocker B4 on PR nearai#3635: the `caller_event_id` threading through `BeforeCapabilityHookContext` partially shipped earlier (commit b4d8a35), but the trust-boundary documentation explaining the host-assigned invariant was still missing. Add rustdoc to `PredicateEventId` and the `PredicateStateBackend` trait clarifying that: - the id MUST be minted by trusted host code from authoritative sources (dispatcher RuntimeEventId, host-side hash, arguments digest) - it MUST NOT pass through unchanged from any tenant-controlled surface (capability arguments, manifest fields, WASM memory, HTTP bodies) - the format invariants in `PredicateEventId::new` (non-empty, NUL-free) are a durability contract for SQL backends, NOT a trust check - a tenant-supplied id can either undercount itself into infinity by replaying a fixed id, or poison adjacent buckets if scoping is ever weakened Doc-only; no behavior change. * test(hooks): add caller-boundary replay-dedup test through wrapper hook henrypark133 HIGH blocker B1 on PR nearai#3635: replay dedup must engage at the caller boundary — `PredicateBackedBeforeCapabilityHook::evaluate` is the production path the dispatcher invokes for installed predicate hooks. A unit test on `PredicateEvaluator::evaluate_at` alone is insufficient regression coverage (repo CLAUDE.md rule "Test through the caller, not just the helper"): the wrapper hook reads `BeforeCapabilityHookContext::caller_event_id` and threads it down to the backend, so the regression test must drive the wrapper itself. The threading work already shipped in commit e6df47d (`caller_event_id` field on the public hook context + evaluator preferring it over the synth path). This commit adds the missing end-to-end test: 1. Two `PredicateBackedBeforeCapabilityHook::evaluate` calls with the same `caller_event_id` and a `RateOrValueCap { max: 1 }` predicate — the second call must stay under cap (dedupe engages at the wrapper boundary, not be re-counted into a deny). 2. A third call with a DISTINCT `caller_event_id` crosses the cap — proving dedup is replay-scoped (same id → no-op), not blanket- suppress (any id → no-op). If the wrapper were synthesizing a fresh id per call (the bug Henry flagged before threading landed), this test would fail at step 2 with the second evaluation being denied. * docs(hooks): D5a + cross-process replay note; add caller-API tests henrypark133 should-fix S8 + S9 on PR nearai#3635. S8 — threat-model expansion: - Add D5a as the correctness-under-attack variant of D5: an attacker flooding high-cardinality keys can LRU-evict legitimate tenants' counters and reset their rate-limit state. Distinct from the memory-only framing of D5; tied back to per-extension caps (D3/D4) and the durable-backend successor (doc 03). - Document the cross-process replay limit on the in-memory backend inside the PredicateStateBackend trait docs, not just in D5 — the process-local dedup is a property callers need at the trait surface, with a pointer to the durable backend as the cross-host story. S9 — three new tests on the in-memory backend public API: - lru_eviction_via_public_api_holds_max_history_keys_cap: drives MAX_HISTORY_KEYS + 1 distinct keys through record_invocation and asserts the map size cap holds + evictions_observed() advances. The previous coverage manually crafted buckets and called the LRU helper directly; this exercises the production path. - in_memory_invocation_retains_entry_at_exact_window_cutoff: pins the `< cutoff` trim semantics so a refactor to `<=` would fail loud. - event_id_dedup_is_isolated_across_invocation_and_value_maps: same event_id used in both record_invocation and record_value must not cross-suppress — the two maps key on disjoint types. The fourth S9 item (concurrent N-thread atomicity) and the caller- boundary replay test on the wrapper hook already landed in earlier commits (f632d22, predicate_state.rs line 840). S2 (evict_older_than stub), S3 (sync-trait docs), and S7 (consistency vs batched-writes) were also already in HEAD; this commit ships the remaining items. Quality gate: cargo fmt clean, cargo clippy -p ironclaw_hooks --all-features --tests -D warnings clean, full hooks test suite green (15 predicate_state unit tests + lib + integration). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(hooks): co-locate synth_event_id with backend; rationale comment; pin synth format henrypark133 nits N1, N2, N5 on PR nearai#3635. N1 — Move `synth_event_id` from `evaluator.rs` to `predicate_state.rs` as `PredicateEventId::synth(...)`. The id format (64-char lowercase hex, no NUL, never empty) is part of the backend's durable contract, so co-locating with `PredicateStateBackend` keeps the format change- surface adjacent to the consumer. To avoid inverting the module dependency (`predicate_state` is a leaf below `points`), the synth helper takes raw bytes / &str rather than a `&BeforeCapabilityHookContext`. The evaluator's `resolve_event_id` fallback unpacks the context and delegates. N2 — `// safety:` comment on a non-`unsafe` block (the `write!(s, "{byte:02x}")` infallibility note) renamed to `// RATIONALE:`. By convention `// SAFETY:` pairs with `unsafe` blocks; using `// safety:` elsewhere conflates the two. N5 — Add `synth_event_id_is_64_char_lowercase_hex` to pin the synth output shape. A refactor that silently changes length or case would break the durable backend's `uuid`-shaped UNIQUE constraint without a test failure today; the new test fails loud. Quality gate: cargo fmt clean, cargo clippy --all --benches --tests --examples --all-features -D warnings clean, full hooks lib test suite green (172 passing including the new pin). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): drop expect() in hex formatting to satisfy panic CI check The "No panics in production code" CI check (scripts/check_no_panics.py) only recognizes `// safety:` suppression markers, not `RATIONALE:`. Since std::fmt::Write for String is infallible, just discard the Result with `let _ =` instead of `.expect()` — no panic call, no marker needed. Also merges in latest origin/hooks-foundation-01 (now includes the reborn-integration merge and PR nearai#3636). * fix(hooks): narrow caller_event_id visibility to pub(crate) henrypark133 MED on PR nearai#3635 5-19 review. The pub field let external callers bypass with_caller_event_id and assign values the validated PredicateEventId constructor would have rejected. Force every external caller through the typed setter so PredicateEventId::new is the only entry point. * fix(hooks): drop arguments_digest from synth + add in-memory backend warn Two PR nearai#3635 5-19 review findings on the evaluator surface: - henrypark133 LOW (synth oracle): drop arguments_digest from the PredicateEventId synth hash input. The 64-char hex output was an equality oracle for argument shape; replay dedup for durable backends uses the caller-supplied caller_event_id, not the synth path, so synth only needs to be per-call unique, not content-addressed. - henrypark133 HIGH + MED (in-memory production limits): expose PredicateEvaluator::warn_in_memory_backend_active_in_production for hosts to call at startup. Multi-host replay dedup is process-local and the LRU cap is shared across tenants; operators need this surfaced in logs when the durable backend is not wired. * fix(hooks): harden predicate state backend per PR nearai#3635 5-19 review Address five findings on crates/ironclaw_hooks/src/predicate_state.rs: - A1 (henrypark133 HIGH): restrict PredicateEventId::new_unchecked to pub(crate) so external callers cannot bypass the durable UNIQUE-constraint format invariants enforced by ::new. - A4 (henrypark133 MED): per-tenant LRU quota at MAX_HISTORY_KEYS / 4. Without it a noisy tenant could fill the global cap and evict a quiet tenant's bucket, resetting their rate-limit counter. With the quota, a tenant that overflows evicts its OWN oldest-front bucket first. New tests cover single-tenant cap and cross-tenant isolation. - A5 (henrypark133 LOW): drop arguments_digest from the synth hash input (oracle closure mirrored from the evaluator side). Add a thread-local nonce alongside the process-global counter so synth remains per-call unique without relying solely on a contended AtomicU64. New test pins the divergence invariant. - D6 (henrypark133 HIGH): O(1) NumericSum via an incrementally- maintained ValueBucket::running_sum, replacing the O(n) deque walk on every record_value call. New test covers push/trim/replay interactions. - D8 (henrypark133 MED): implement evict_older_than for the in-memory backend (was a no-op Ok(0) default). Drops entries strictly older than the cutoff and removes empty buckets; operator reaper tasks rely on this to reclaim memory from idle keys. - D7 (henrypark133 MED, partial): document the process-global synth COUNTER as a known contention hotspot and add a thread-local nonce so threads can advance without forcing cross-core invalidation in the common path. Tests: 196 passing (+4 new); workspace clippy clean. * fix(hooks): port MAX_SAMPLES_PER_KEY cap into PredicateStateBackend (D5 regression from r3) Round 3 of PR nearai#3573 (already merged into hooks-foundation-01) added an inline per-key sample cap of 4_096 in evaluator.rs to bound memory under attacker-triggered hot capabilities with very large declared windows (threat-model finding D5). The predicate-state extraction in PR nearai#3635 moved that bookkeeping into the PredicateStateBackend trait but missed porting the cap, so the cap would silently disappear from production once this PR rebases onto the foundation branch. This commit moves the cap into the in-memory backend impl next to MAX_HISTORY_KEYS / MAX_KEYS_PER_TENANT and enforces it in both record_invocation and record_value. For the NumericSum path, the bucket helper's pop_front already decrements running_sum, so the incremental sum invariant survives cap-driven eviction. The pre-existing inline copy in evaluator.rs becomes redundant once the trait impl owns the enforcement; the rebase resolution deletes it. Adds two regression tests: - record_invocation_caps_samples_per_key_under_attacker_pressure - record_value_evicts_oldest_keeping_running_sum_consistent * fix(hooks): port predicate_state tests to ::new() after nearai#3912 newtype privatization --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
… to nearai#3573) (nearai#3637) * docs(hooks): scope invocation_arguments_digest snapshot pin (successor #10) Successor PR from nearai#3573 — addresses serrrfirat's #3 follow-up. Adds an explicit snapshot test pinning the digest for a known input so a future change to the hashing path or input-ref format is loud. * test(hooks): pin invocation_arguments_digest with snapshot Address serrrfirat's #3 follow-up from PR nearai#3573 — partial fix promoted to full pin. Two new tests in capability_port.rs::tests: - invocation_arguments_digest_is_stable_for_known_inputs: pins the digest for a fixed (capability_id, input_ref) fixture so any future change to the hashing structure or input-ref format is loud. The captured hex is documented in the assertion message + stability contract. - invocation_arguments_digest_differs_for_different_input_refs: structural sanity check that distinct inputs produce distinct digests. Plus expanded rustdoc on the function calling out the stability contract: changing the hashing path requires updating the fixture, surfacing in the cross-crate wire-format contract section, and bumping the framework's contract version if downstream consumers exist. Tests: 156 unit tests pass (was 154; +2 new). Clippy + fmt clean. * test(hooks): pin arguments_digest at middleware boundary (serrrfirat nearai#3637) serrrfirat MED on PR nearai#3637: the existing snapshot test pins `invocation_arguments_digest`'s raw output, but the public hook contract is `BeforeCapabilityHookContext.arguments_digest` populated via `HookedLoopCapabilityPort::hook_context`. If caller-side wiring drifts — wrong field set, transform inserted, stale/default digest, or an alternate path bypassing the helper — the helper snapshot stays green while hook consumers observe a broken digest. Add a boundary-level pin: construct a `HookedLoopCapabilityPort` with a no-op inner port and an empty dispatcher, run the same fixed `(capability_id, input_ref)` invocation through `hook_context()`, and assert the resulting `ctx.arguments_digest` matches the same pinned hex as the helper snapshot. If they ever disagree, this assertion fails — surfacing wiring drift that the helper test alone cannot. `HookedLoopCapabilityPort::hook_context` is widened from private to `pub(crate)` to make the boundary test possible without bypassing the function. * docs(hooks): clarify arguments_digest rustdoc — input-ref identity, not arguments (serrrfirat blocker on PR nearai#3637) The rustdoc summary on invocation_arguments_digest described the digest as covering "capability arguments" with equivalence under "identical arguments". This contradicted the new stability section in the same file, which (correctly) documents that the digest is over the (capability_id, input_ref) identity tuple — NOT over the resolved argument content the input-ref points at. Reword the summary and the equivalence claim to describe input-ref identity, matching the stability section and the actual implementation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…nearai#3640) * docs(hooks): scope event-triggered hooks (Phase 5, successor #4) Successor PR from nearai#3573. Adds a new EventTriggered hook point that subscribes to RuntimeEvents asynchronously, outside the loop's inline tick. Observer-only by construction (no Allow/Deny/Patch); typed against a narrowed HookObservableEvent projection to keep the cross-crate boundary clean. Scope doc only; design questions about cursor/replay semantics and per-extension event-rate caps need design review before implementation. * Implement Phase 5 event-triggered hooks Cites crates/ironclaw_hooks/docs/successors/04-event-triggered-hooks.md as the scope contract. Adds the EventTriggered observer hook point, durable RuntimeEvent dispatch path, and Reborn pull-driven subscription wiring with caller-level coverage for matching, replay, scope filtering, observer-only authority, and backpressure. * Fix hook event OwnCapabilities owner lookup * fix(hooks): carry owning extension into hook milestone runtime events henrypark133 HIGH + codex P1 on PR nearai#3640: `OwnCapabilities`-scoped event-triggered subscriptions silently never fired for `HookFailed`/`HookDecisionEmitted`/`HookDispatched` events because those `RuntimeEvent` constructors hardcoded `provider: None`. Since Installed hooks default to `OwnCapabilities`, the very events that Phase 5 was designed to observe (hook-failure / decision alerting) never reached their default-configured subscriber. A prior fix added a hook_id-based fallback in `scope_provider_for_runtime_event` that resolves the owning extension through the registry's hex index when `event.provider` is `None`. That covers the case where the failing hook is still registered at replay time, but the durable fix is to stamp the originating provider into the event at emit time so the primary `event.provider` path resolves without any fallback. Plumbed `owning_extension: Option<ExtensionId>` end-to-end: - `LoopHostMilestoneKind::{HookDispatched, HookDecisionEmitted, HookFailed}` gain the field (with `#[serde(default, skip_serializing_if = "Option::is_none")]` so pre-existing checkpoint payloads and the L3 schema-snapshot tests round-trip unchanged when no owner is set). - `RuntimeEvent::hook_{dispatched, decision_emitted, failed}` constructors accept the owner and stamp it into `provider`. - `milestone_events.rs` threads the field through the projection. - `HookDispatcher::emit_dispatched/emit_decision` pass `binding.owning_extension.clone()` directly. - `HookDispatcher::emit_failure` (no binding handy on the failure path) looks the owner up via the registry's existing `owning_extension_for_hook_hex` index. Tests: - `event_triggered_own_capabilities_matches_hook_failed_with_carried_provider`: primary-path regression — two `HookFailed` events with `provider: Some(ext_a|ext_b)` against an `OwnCapabilities` subscription scoped to ext_a; only the own-provider event fires and `event.provider == Some(ext_a)`. - Existing `event_triggered_own_capabilities_scope_resolves_hook_failed_owner_from_hook_id` remains green: passes `None` for the new arg so the fallback path is still exercised for legacy payloads. All other call sites updated to pass `None` (no owner available) or the resolved owner where applicable. * fix(hooks): validate event subscription scope against run scope (serrrfirat HIGH #1 on PR nearai#3640) `EventTriggeredHookSubscription` accepted a caller-supplied `EventStreamKey` + `ReadScope` and used `run_context.scope.tenant_id` as the hook context's tenant — with no validation that the two agreed. A caller wiring tenant A's host with tenant B's stream would cause hooks to observe B's events while the hook context claimed tenant A. Cross-tenant trust-boundary break. Add `EventTriggeredHookSubscription::validate_against_run_scope` and call it from `build_text_only_host_with_capabilities` before spawning. Validation: - Stream `(tenant_id, user_id, agent_id)` must equal `(run_context.scope.tenant_id, thread_scope.owner_user_id, run_context.scope.agent_id)`. - Thread without `owner_user_id` cannot bind any subscription — the user dimension is required to verify stream identity. - Every `Some(want)` in `ReadScope` must equal the corresponding run/thread scope value (project/mission/thread). `None` is permissive (run scope owns the dimension authoritatively). Failures surface as `RebornLoopDriverHostError::ScopeMismatch` with a specific reason naming the offending dimension. Tests: - `event_triggered_subscription_with_foreign_tenant_stream_fails_host_build` - `event_triggered_subscription_with_foreign_user_stream_fails_host_build` The integration fixture's `ThreadScope` now sets `owner_user_id: Some(...)` so it passes validation; previously it was `None`, which the new check (correctly) refuses. Existing tests continue to pass. * fix(hooks): surface event subscription replay-gap as a milestone (serrrfirat MED on PR nearai#3640) When the durable event log returned `EventError::ReplayGap`, the event-triggered subscription's background task previously logged a `tracing::warn!` and broke out of the poll loop — silently killing all future hook event delivery for the run with no operator-visible signal. A scoped audit hook that mattered to compliance would just stop, and nobody downstream would know. Surface the termination through the host's milestone sink: - New `LoopDriverNoteKind::EventSubscriptionTerminated` variant. - The subscription's `spawn`/`run` now takes the host's `Arc<dyn LoopHostMilestoneSink>` and the active `LoopRunContext`. On `ReplayGap`, it constructs a `DriverNote` milestone with that kind plus a `LoopSafeSummary` describing the gap, publishes it through the same sink that carries every other host milestone, and *then* breaks (fail-closed: the at-most-once contract is already broken; resuming from `earliest` would silently lose the gap). - Log level bumped from `warn` to `error` to match the severity. - A best-effort send: failures to publish the milestone are logged but do not stall the subscription teardown. Tests: - `event_triggered_replay_gap_emits_subscription_terminated_milestone`: appends 3 events, `truncate_before_or_at` to cursor 2 to force a replay gap, starts the subscription from cursor origin (now stale), and asserts a `DriverNote { kind: EventSubscriptionTerminated, .. }` shows up on the host's milestone sink within a 2s deadline. Self-emit reentrancy (serrrfirat MED #3 on the same PR) is intentionally not addressed here — that fix needs a design call (task-local re-entry flag vs. removing RuntimeEvent emit capability from event-hook execution contexts) and is a follow-up. * fix(hooks): suppress event-triggered self-observation (serrrfirat MED #3 on PR nearai#3640) A hook that subscribes to one of the hook-lifecycle event kinds (`HookDispatched`/`HookDecisionEmitted`/`HookFailed`) with a scope that matches its own provider would otherwise be dispatched for events describing its OWN executions. The dispatcher emits those events itself when running the hook, so a hook subscribing to `HookFailed` with `OwnCapabilities` against its own extension would fail → emit HookFailed → re-dispatch → fail → emit → … storm. `dispatch_event_triggered_at` now skips events whose `event.hook_id` equals the binding's own hook id when the event kind is a hook- lifecycle kind (`is_hook_lifecycle_kind`). The check is intentionally narrow: - It only fires for hook-lifecycle events. Subscriptions to other event kinds are unaffected. - It only suppresses literal self-observation; events about other hooks (even hooks from the same extension) still dispatch. This does NOT cover the broader case of a hook that captures an `Arc<DurableEventLog>` and mints arbitrary `RuntimeEvent`s from inside its `observe()`. That requires architectural restriction on what hook impls can capture — tracked separately as a follow-up. Tests: - `event_triggered_self_lifecycle_event_does_not_redispatch`: appends two `HookFailed` events with the same provider — one targeting the subscriber's own hook id, one targeting a different hook. Asserts only the OTHER hook's failure fires (proves the filter is narrow, not blanket). * fix(hooks): address henrypark133 should-fix #1, #2, #3, #6 on PR nearai#3640 Four items from the 5-15 review (#4 DoS budget and #5 narrowed projection deferred — see below): **#1 (should-fix) Invariant: EventTriggered ↔ event_kind_filter** `HookRegistry::insert` now enforces the biconditional at install time: an `EventTriggered` binding must declare an `event_kind_filter` (otherwise the dispatcher's kind match would silently never fire — a no-op binding), and conversely only `EventTriggered` bindings may declare a filter (other points are kind-agnostic and would ignore the field). Misconfigured bindings fail loud at install. **#2 (should-fix) Remove `Clone` derive on EventTriggeredHookSubscription** `Clone` on a spawn-semantics type was a footgun: external callers cloning + spawning twice would create two consumers reading from the same `start_cursor`, each dispatching every hook. Replace with an explicit `clone_for_independent_spawn(&self)` method named verbosely so the property is visible at the seam. Internal use updated in the factory's host-build path; external callers can no longer accidentally construct a dual-consumer pattern. **#3 (should-fix) catch_unwind around the background `run()` task** The subscription's tokio task body now runs inside `AssertUnwindSafe(...).catch_unwind()`; a panic in `run()` emits the same `EventSubscriptionTerminated` `DriverNote` milestone the `ReplayGap` path already emits, instead of silently terminating with no operator-visible signal. **#6 (should-fix) Replay semantics in rustdoc on public API** Added a "Replay semantics" section to `EventTriggeredHookSubscription` rustdoc: at-least-once, caller-owned cursor persistence, the restart-from-start_cursor replay pattern. Previously only in the design doc; now load-bearing API contract is visible at the type. **#4 (deferred) Per-hook DoS budget for Installed tier** Henry's recommendation was to gate `Installed`-tier event-triggered hooks entirely until the budget design lands, allowing only Builtin/Trusted. That breaks 11 existing tests + the primary use case. Instead: documented the existing first-line throttle (`batch_limit` × `poll_interval`) as the current bound on indirect- recursion fanout, and tracked the full per-hook rate cap with poisoning + milestone-on-overrun as a follow-up. The self-trigger guard (committed earlier in this PR) catches the most common direct pattern; the throttle here bounds the indirect pattern until the proper budget lands. **#5 (deferred) Narrowed `HookObservableEvent` projection** Would prevent full `RuntimeEvent` surface from reaching Installed- tier hooks. Project-wide impact (events crate types, projection glue). Tracked as a follow-up; the existing sanitized-event projection bounds the surface to closed-vocab labels. All 156 hooks lib + 30 reborn integration tests pass. * chore(hooks): address nits from PR nearai#3640 review Bundle three nit-tier review items into a single commit: **#9 Replace author-internal tags with NOTE(nearai#3640)** The Phase-5 PR (nearai#3640) had several `serrrfirat HIGH/MED #N on PR nearai#3640` comment tags in this PR's diff. These are review-internal scaffolding, not load-bearing for future readers. Replaced with `NOTE(nearai#3640)` in: - crates/ironclaw_hooks/src/dispatch.rs (self-observation guard) - crates/ironclaw_reborn/src/loop_driver_host.rs (scope validation, replay-gap milestone, subscription binding) - crates/ironclaw_reborn/tests/hooks_integration.rs (three regression tests covering scope validation, self-observation suppression, and replay-gap surfacing) - crates/ironclaw_turns/src/run_profile/host.rs (`EventSubscriptionTerminated` doc) **#10 Replace 10ms spin-poll with tokio::sync::Notify** `wait_for_seen_events` polled the shared `Mutex<Vec<SeenRuntimeEvent>>` every 10 ms until the expected count was reached. Replaced with a `SeenLog` newtype that pairs the events vec with a `Notify`; the hook's `observe()` calls `seen.push(...)` which signals `Notify::notify_one`, and `wait_for_seen_events` parks on `notified().await` under a `tokio::time::timeout`. `notify_one` is a permit-store, so an event landing between snapshot and wait still wakes the waiter immediately. Test latency drops from ~10 ms median to sub-ms and is no longer rate-limited by the polling cadence. All 30 hooks_integration tests still pass. **#11 Remove unused Clone derive on EventTriggeredHookContext** No call site clones the context — it's passed by reference. Dropped the derive to make the borrow contract clearer. * docs(hooks): reconcile event-triggered hooks design doc with Phase 5 reality Address gemini-code-assist review on `04-event-triggered-hooks.md`: - L50 (Likely surface): annotated the sketch's full `RuntimeEvent` use with a pointer to the narrowed-projection follow-up so the snippet no longer reads as a recommendation contradicting L119–121. - L55 (sink methods): replaced `note_fact` / `emit_audit` (which never shipped on `ObserverSink`) with the actual `note(category, summary)` primitive and cross-referenced Reborn's `EventTriggeredObserverSink`. - L95 (cursor / replay): "lost events during downtime acceptable" contradicted the at-least-once replay semantics described in the Phase 5 implementation notes. Rewrote the bullet to say replay is at-least-once from the persisted cursor and to spell out the operator obligation around cursor persistence before shutdown. - L100/115 (forbids events dep): the original doc claimed `ironclaw_hooks` forbids an `ironclaw_events` dep, but the Risk section noted the dep is already established via PR nearai#3573. Updated both passages to reflect that the dep direction is set; Phase 5 adds the *consumer* side. The narrowed `HookObservableEvent` projection is now framed as a follow-up tracked in nearai#3690. * refactor(hooks): unify event-triggered sink with ObserverSink Address PR nearai#3640 review findings A3, C4, F14, and cluster G: - F14: drop duplicate `EventTriggeredObserverSink` trait and reuse `ObserverSink` directly in the `EventTriggeredHook` trait. The two surfaces were signature-identical; keeping them separate let them drift, and a future gate/mutator method added to one would not surface as a compile error on the other. - A3: add `is_replay: bool` to `EventTriggeredHookContext` and a dedicated `dispatch_event_triggered_replay_at` entry point. The subscription contract is at-least-once, so side-effecting hooks need to dedupe by `event.event_id` on restart-driven replay. - C4: index event-triggered bindings by `RuntimeEventKind` at install time so dispatch is O(matches) instead of scanning every event-triggered binding for every event. - Cluster G: doc/04-event-triggered-hooks.md updated to reflect the unified sink, the explicit at-least-once semantics + `is_replay` signal, the actual `note(category, summary)` primitive (not the speculative `note_fact` / `emit_audit`), the corrected `ironclaw_events` dep status, and the issue nearai#3690 reference for the narrowed `HookObservableEvent` projection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * perf(hooks): adaptive backoff for event-triggered subscription Address PR nearai#3640 review findings C5, A1, A2: - C5: empty-poll backoff for `EventTriggeredHookSubscription`. The previous loop hammered the durable log at a fixed 50ms cadence under sustained idle, even when no events had arrived for minutes. The subscription now tracks consecutive empty polls and sleeps for `min(poll_interval << streak, max_poll_interval)` before the next poll, defaulting to a 1s cap; a non-empty batch resets the streak so producer bursts restore low-latency dispatch immediately. Exposed via `with_max_poll_interval` so callers can tune. - A1 / A2: explicit issue references for the deferred narrowed `HookObservableEvent` projection (nearai#3690) and the per-hook DoS dispatch budget (nearai#3689). The current self-trigger guard catches direct-recursion storms; the backoff bounds indirect ones until the proper budget design lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(hooks): cover event-triggered dispatch edge cases Address PR nearai#3640 review findings D8, D9, D10, D11, D12, and B: - D8: dispatching an event-triggered binding that has no installed hook impl must poison the slot and surface a Malformed failure rather than silently no-op. A follow-up dispatch on the same kind must skip the poisoned slot. - D9: registry validation rejects non-event-point bindings that carry an `event_kind_filter`, mirroring the existing reverse-direction check. - D10: the existing hook-meta serde round-trip tests always passed `None` for `owning_extension` and never asserted `event.provider`. Add `hook_meta_events_round_trip_owning_extension_as_provider` to pin the projection that scope filtering depends on. - D11: `scope_provider_for_runtime_event` falls back to `None` when the registry mutex is poisoned. Force a poison on a spawned thread and assert the resolver remains fail-closed. - D12: `run_event_triggered_hook` catches panics from the hook impl via `AssertUnwindSafe::catch_unwind`. Drive it with a deliberately panicking impl and assert `FailureCategory::Panic`. - Cluster B: when a hook-meta event has `provider: None`, the dispatcher recovers the owning extension from the registry's hex-keyed index so `OwnCapabilities` watchers still fire. Add a full end-to-end test exercising that path through `dispatch_event_triggered_at`. Also pin C4 indexing: a registry-level test that `active_for_event_kind` returns only bindings whose declared filter matches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): port event-triggered tests after foundation rebases (nearai#3911/nearai#3912/nearai#3913) - Add event_kind_filter: None to HookBinding test constructions (foundation added new field) - Replace tuple-struct construction of ExtensionId/HookLocalId with ::new() per nearai#3912 newtype privatization - Lowercase RuntimeEventKind debug repr for HookLocalId validation (lowercase-only identifiers) - Replace pub-use re-exports with module-path imports per foundation cleanup - Rename fixture.user_id to fixture.actor_id per nearai#3633 final naming * fix(hooks): adapt event-triggered to WASM hook runtime (nearai#3920) - Add event_kind_filter: None to WASM HookBinding constructions in dispatch.rs - Extend HookManifestKind match arms in registrar.rs and wasm/runtime.rs to handle EventTriggered (rejected: WASM-bodied event-triggered hooks are not yet supported) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…ED flag (nearai#3934) (nearai#3938) * feat(hooks): extension-declared hook section on ExtensionManifestV2 (nearai#3934) Add a `[[hooks]]` declaration surface to the production v2 extension manifest (`ironclaw_extensions::ExtensionManifestV2` and its projected `ExtensionManifest`). Each entry is carried as a structurally-typed `HookSectionEntryV2` DTO — a `local_id` plus the entry's body re-serialized to canonical TOML — so `ironclaw_extensions` (substrate) never imports the `ironclaw_hooks` predicate vocabulary. The composition layer, which depends on both crates, is the single seam that projects these payloads into typed `ironclaw_hooks::HookManifestEntry` values (a later commit). Parse-time structural bounds: `MAX_MANIFEST_HOOKS` (32, matching the downstream per-extension registration cap) and `MAX_HOOK_ENTRY_BYTES` (8 KiB per entry). Entries must be tables carrying a non-empty `id`; ids must be unique within the manifest. `#[serde(default)]` keeps every existing manifest valid (empty `hooks` vec). The DTO holds canonical TOML as a `String` rather than a `toml::Value` so the enclosing `ExtensionManifestV2` keeps its `Eq` derive (`toml::Value` is not `Eq`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(hooks): composition-layer activation module (loader, first-party hook, flag) (nearai#3934) Add `ironclaw_reborn_composition::hooks` — the single seam that activates the hook framework in production. Implements four numbered pieces of nearai#3934: - Feature flag (item 7): `HooksActivationConfig`, default OFF, resolved from `HOOKS_ENABLED` (only `1`/`true`/`yes`/`on` enable; unset/anything else = OFF). Flag OFF ⇒ `build_hook_dispatcher_builder_factory` returns `None` and the runtime composes no dispatcher — exact pre-hooks behavior. Hard rollout-safety contract. - Manifest → registry loader (item 2): `install_extension_hooks` projects each `ExtensionManifestV2` `HookSectionEntryV2` (canonical TOML) into a typed `HookManifestEntry` and installs it via `HookRegistrar::install` at the `Installed` trust tier. This is the clean-boundary projection: the hook vocabulary lives only here, never in `ironclaw_extensions`. Trust attenuation is enforced by construction (registrar only calls `install_installed_*`). Fail-closed: any projection/install error fails the build loudly. - First-party builtin hooks (item 3): a single illustrative no-op observer (`NoOpObserverHook`), installed regardless of extensions. Ships dark (zero driver-visible effect even with the flag ON). Production catalog is TBD by design — this PR does not invent a first-party hook. - Dispatcher composition (item 5): builds a per-tenant `PredicateEvaluator` over the in-memory state backend (swappable via the new public `PredicateEvaluator::with_state_backend` for durable nearai#3933), validates the full install set once fail-closed, and returns a per-run builder-factory closure. Per-run construction (fresh registry/dispatcher per host build) + per-tenant evaluator give full isolation; the host factory attaches the run-scoped milestone sink internally. Per-tenant scoping is by construction: `build_reborn_runtime` runs once per identity, so everything here is tenant-local — no global registry. The router-backed gate-ref factory (PauseApproval/PauseAuth) and the security-audit sink (nearai#3922, not yet on this branch) are deferred follow-ups; their absence is fail-closed (PauseApproval surfaces as Denied) and noted for the PR body. Not yet wired into the runtime — next commit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(hooks): wire dispatcher builder factory into build_default_planned_runtime (nearai#3934) Item 6 of nearai#3934. Add an optional `hook_dispatcher_builder_factory` to `DefaultPlannedRuntimeParts` and, in `build_default_planned_runtime`, call `.with_hook_dispatcher_builder_factory(...)` on the production `RebornLoopDriverHostFactory` when it is present. `None` (the default) means no dispatcher is composed — behavior identical to the pre-hooks runtime (rollout-safety contract). The composition layer (`build_reborn_runtime`) resolves the flag via `HooksActivationConfig::from_env()` and builds the factory against this tenant's extension registry (per-tenant by construction — the function runs once per identity). Fail-closed: a malformed manifest hook fails the build here rather than composing a broken dispatcher. A per-run builder factory (not a captured dispatcher instance) is used so the host attaches a run-scoped milestone sink internally per build — per-run telemetry attribution, the nearai#3573 capture-and-stick lesson. All `DefaultPlannedRuntimeParts` construction sites (8 test sites across ironclaw_reborn, ironclaw_product_workflow, ironclaw_reborn_composition) updated with the new field defaulting to `None`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): e2e activation tests through build_default_planned_runtime (nearai#3934) Item 8 of nearai#3934. Add four end-to-end tests in crates/ironclaw_reborn/tests/loop_driver_host.rs that drive the *production* composition function `build_default_planned_runtime` with a per-run hook dispatcher builder factory shaped exactly like the composition layer's output (first-party builtin no-op observer + extension-declared `Installed`-tier hooks projected from a manifest entry through `HookRegistrar::install`), then build a host via the composed `host_factory` and invoke a capability: - flag OFF (no factory): allowed capability completes unaffected and reaches the inner host runtime port — the pre-hooks behavior / rollout-safety contract. - flag ON, first-party-only no-op observer: outcome unchanged, inner port reached — the builtin ships dark. - flag ON, extension-declared deny hook: capability denied through the composed runtime and the inner port is never reached (installed at the Installed tier via the registrar; OwnCapabilities scope keyed to the capability provider). - per-tenant isolation: tenant A's deny hook fires; tenant B (separate build_default_planned_runtime composition, no hooks) completes the same capability — proving no cross-tenant leakage. Security-audit-on-deny assertion is intentionally deferred: nearai#3922's SecurityAuditSink is not yet on reborn-integration. It lands with the audit-sink wiring follow-up. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): thread hook_dispatcher_builder_factory through shared reborn harness (nearai#3934) The root-crate `tests/support/reborn/harness.rs` constructs `DefaultPlannedRuntimeParts` directly; add the new `hook_dispatcher_builder_factory: None` field so the parity-test harness compiles. Default `None` keeps the harness on the no-hooks path (unchanged behavior). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks): proper error handling / safety annotations for activation production paths (nearai#3938 CI) The per-run dispatcher factory closure used `.expect()` on the first-party and extension hook installs, tripping the no-panics CI gate. These installs are pure replays of the install set already validated fail-closed (`?`) against a scratch builder at composition time, so they are genuine invariants. The factory type returns a non-Result `HookDispatcherBuilder` and is invoked deep in the run loop, so the documented `// safety:` suppression is the correct fix here. Hoisted the expect messages into `let` bindings so the `.expect(msg)` call fits on one line, keeping the scanner-required `// safety:` comment on the same line as the call after rustfmt. The malformed-manifest path (TOML projection) already uses real error propagation via map_err/`?` and is unaffected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): direct composition-loader coverage + activation-scope docs Address Codex non-blocking follow-ups on nearai#3938. Add three direct tests for the composition-layer hook loader (`install_extension_hooks` via `build_hook_dispatcher_builder_factory`), driving a real `ExtensionRegistry` with `[[hooks]]` declarations rather than mimicking the loader: - valid `own_capabilities` predicate hook installs at the Installed trust tier; the dispatcher carries the derived binding at BeforeCapability alongside the first-party no-op observer - malformed typed hook body (unknown `mode`) fails CLOSED with `RebornBuildError::InvalidConfig`, never a panic (the load-bearing degradation contract for untrusted external manifests) - a hook claiming `scope = same_tenant` without a verified grant is rejected by trust attenuation (fail-closed) No loader bug surfaced: `HookRegistrar::install` already returns `Result` on every malformed/over-scoped path and the loader maps it to `InvalidConfig` via `?`. Document activation scope at both the loader rustdoc and the `build_reborn_runtime` call site: production currently passes only `builtin_extension_registry()`, so third-party installed-extension hooks are not yet surfaced into the runtime path — only first-party-builtin and builtin-package-declared hooks activate today. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hooks): thread HooksActivationConfig through input; empty production catalog Two maintainability cleanups on nearai#3938 (firat review): Item 1 — config hygiene: stop reading HOOKS_ENABLED via std::env::var deep inside build_reborn_runtime. Add a typed `hooks: HooksActivationConfig` field to RebornRuntimeInput (default OFF) plus a `with_hooks_config` builder. The composition root now consumes the typed config; the env var is resolved ONCE at the edge (the reborn CLI's build_runtime_input) via HooksActivationConfig::from_env and threaded down. Testable without env mutation; matches the project's env → typed config → composition pattern. Item 2 — empty production first-party catalog: stop shipping NoOpObserverHook as a first-party builtin. install_first_party_hooks is now a no-op (empty catalog); the production type/install/export for a hook that does nothing is gone (removed from lib.rs exports). The activation machinery is still tested end-to-end through the real composition path via a new `build_hook_dispatcher_builder_factory_with` seam that takes a first-party installer; tests pass a `#[cfg(test)]` NoOpObserverHook through it. Pinned the empty-catalog-is-valid contract: flag ON + empty first-party set + no extension hooks composes a valid zero-binding dispatcher (not a panic/error). Reconciled the sibling loader tests/docs (f6c79c0): the in-module tests now drive the test-only seam; the reborn e2e tests already used a test-local no-op and are untouched. Updated activation-scope docs (loader rustdoc + build_reborn_runtime call site) to reflect the now-single live source (builtin-package-declared hooks). Deferred (not touched): switching to the canonical extension registry for third-party installed-extension hooks (nearai#3934 follow-on). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hooks): canonical registry + infallible plan + tenant-scoped counter docs/tests (nearai#3938) Addresses serrrfirat's thermo-nuclear re-review on 1e618d0. #1 (runtime.rs:839, canonical registry): make the extension registry a shared composition artifact. `build_local_dev` builds one `Arc<ExtensionRegistry>`, hands it to `HostRuntimeServices::new` AND stores it in `RebornLocalRuntimeServices.extension_registry`. Hook activation in `build_reborn_runtime` now consumes that same `Arc` instead of rebuilding a builtin-only sidecar, so capability dispatch and hook activation cannot drift. Third-party activation stays a follow-up, but it now follows the canonical registry rather than a separate path. #3 (hooks.rs factory machinery): replace the parse/validate/replay duplication + two prose-justified `.expect()` calls with a typed `HookInstallPlan`. TOML is projected once into typed entries, the full install set is validated once against a fresh builder (fail-closed via `?`), and `HookInstallPlan::rebuild` mints a fresh builder per run. The per-run path is infallible by construction: a plan only exists for an install set that already composed cleanly, so a deterministic replay from the identical fresh-empty start cannot fail. One extension-install code path (`project_extension_install_sets` + `install_extension_sets`) is shared by validation and rebuild. #4 (hooks.rs:295, predicate counter scoping): the evaluator/backend is intentionally tenant-scoped and shared across runs (rate/value caps keyed `(hook, tenant, capability)` with no run_id; a run-scoped limit would reset every run and enforce nothing). Document the split explicitly — per-run-fresh dispatcher, tenant-scoped predicate counters — in the module docs and fix the misleading "per-run isolation of hook state" wording in `ironclaw_reborn` (loop_driver_host.rs / runtime.rs). Add `predicate_counter_state_is_tenant_scoped_across_rebuilds`, which drives a real rate-cap predicate through two dispatchers from one factory and proves the second run sees the first run's recorded count. Rename `factory_mints_independent_dispatchers_per_call` -> `rebuild_mints_independent_dispatchers_per_call` and scope it to proving dispatcher freshness only. #6 (loop_driver_host tests): clarify that the hand-built builder factories cover host PLUMBING, not composition activation. Add `build_reborn_runtime_activates_hooks_through_real_composition_path`, which drives the real `build_reborn_runtime` with `HooksActivationConfig` threaded through `RebornRuntimeInput` (env-free) and the canonical registry, proving the production activation wiring composes. #2 (env boundary) and #5 (empty production catalog) were already fixed in 1e618d0; docs touched here for consistency. Known follow-up (not one of the six items, not introduced here): with the flag ON the standalone local-dev runtime does not yet reach `Completed` for a capability turn even with a zero-binding dispatcher — the composition root wires the dispatcher but not the companion hooked-prompt dependencies. The new runtime test asserts `is_terminal()` + the capability path and documents the gap. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): cover hook-entry rejection branches; document InvocationCount cap semantics (nearai#3938) Address henrypark133 review (review 4367870023): - Add extension-manifest tests for the three previously-uncovered hook-entry validation branches: non-table `[[hooks]]` element, whitespace-only `id`, and oversized entry (HookEntryTooLarge). - Document the InvocationCount inclusive-allow / deny-on-overflow semantics inline at the comparison site; behavior unchanged and still pinned by the cap test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(reborn-cli): assert hooks config threaded in caller test (nearai#3938) Addresses the review finding that `build_runtime_input_maps_configured_cli_identity` exercised `build_runtime_input` but never asserted the `hooks` field, so a regression silently dropping `with_hooks_config(HooksActivationConfig::from_env())` or flipping the default-OFF rollout-safety contract would pass. Per `.claude/rules/testing.md` ("Test Through the Caller"): add two assertions to the existing caller-level test: - threading: `runtime_input.hooks == HooksActivationConfig::from_env()`, proving the env-resolved config is actually threaded through and not dropped. Verified via TDD that dropping the wiring fails this assertion (run under HOOKS_ENABLED=1). - default-OFF: when `HOOKS_ENABLED` is unset, `!runtime_input.hooks.is_enabled()`, guarded to skip if the CI environment exports the flag so it only pins the contract it claims to. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(hooks): correct in-memory backend warning text; delegate test ctor (nearai#3938) Address serrrfirat review (2026-06-03): - Low: the in-memory backend warning claimed the LRU cap is shared across tenants, but the Reborn composition constructs a fresh InMemoryPredicateStateBackend per tenant. Rewrite the warn! text and doc comment so the real limitation (process-local replay dedup for multi-host deployments) is accurate, and note the backend is per-tenant in this composition. - Nit: PredicateEvaluator::with_backend (test-only) and with_state_backend had identical bodies; delegate with_backend to with_state_backend so they stay in lockstep. The Medium finding (hooks_config assertion in build_runtime_input caller test) was already addressed in 218a1de. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…ection (HOOKS_THIRD_PARTY_ENABLED, default OFF) (nearai#3951) * feat(hooks): extension-declared hook section on ExtensionManifestV2 (nearai#3934) Add a `[[hooks]]` declaration surface to the production v2 extension manifest (`ironclaw_extensions::ExtensionManifestV2` and its projected `ExtensionManifest`). Each entry is carried as a structurally-typed `HookSectionEntryV2` DTO — a `local_id` plus the entry's body re-serialized to canonical TOML — so `ironclaw_extensions` (substrate) never imports the `ironclaw_hooks` predicate vocabulary. The composition layer, which depends on both crates, is the single seam that projects these payloads into typed `ironclaw_hooks::HookManifestEntry` values (a later commit). Parse-time structural bounds: `MAX_MANIFEST_HOOKS` (32, matching the downstream per-extension registration cap) and `MAX_HOOK_ENTRY_BYTES` (8 KiB per entry). Entries must be tables carrying a non-empty `id`; ids must be unique within the manifest. `#[serde(default)]` keeps every existing manifest valid (empty `hooks` vec). The DTO holds canonical TOML as a `String` rather than a `toml::Value` so the enclosing `ExtensionManifestV2` keeps its `Eq` derive (`toml::Value` is not `Eq`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(hooks): composition-layer activation module (loader, first-party hook, flag) (nearai#3934) Add `ironclaw_reborn_composition::hooks` — the single seam that activates the hook framework in production. Implements four numbered pieces of nearai#3934: - Feature flag (item 7): `HooksActivationConfig`, default OFF, resolved from `HOOKS_ENABLED` (only `1`/`true`/`yes`/`on` enable; unset/anything else = OFF). Flag OFF ⇒ `build_hook_dispatcher_builder_factory` returns `None` and the runtime composes no dispatcher — exact pre-hooks behavior. Hard rollout-safety contract. - Manifest → registry loader (item 2): `install_extension_hooks` projects each `ExtensionManifestV2` `HookSectionEntryV2` (canonical TOML) into a typed `HookManifestEntry` and installs it via `HookRegistrar::install` at the `Installed` trust tier. This is the clean-boundary projection: the hook vocabulary lives only here, never in `ironclaw_extensions`. Trust attenuation is enforced by construction (registrar only calls `install_installed_*`). Fail-closed: any projection/install error fails the build loudly. - First-party builtin hooks (item 3): a single illustrative no-op observer (`NoOpObserverHook`), installed regardless of extensions. Ships dark (zero driver-visible effect even with the flag ON). Production catalog is TBD by design — this PR does not invent a first-party hook. - Dispatcher composition (item 5): builds a per-tenant `PredicateEvaluator` over the in-memory state backend (swappable via the new public `PredicateEvaluator::with_state_backend` for durable nearai#3933), validates the full install set once fail-closed, and returns a per-run builder-factory closure. Per-run construction (fresh registry/dispatcher per host build) + per-tenant evaluator give full isolation; the host factory attaches the run-scoped milestone sink internally. Per-tenant scoping is by construction: `build_reborn_runtime` runs once per identity, so everything here is tenant-local — no global registry. The router-backed gate-ref factory (PauseApproval/PauseAuth) and the security-audit sink (nearai#3922, not yet on this branch) are deferred follow-ups; their absence is fail-closed (PauseApproval surfaces as Denied) and noted for the PR body. Not yet wired into the runtime — next commit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(hooks): wire dispatcher builder factory into build_default_planned_runtime (nearai#3934) Item 6 of nearai#3934. Add an optional `hook_dispatcher_builder_factory` to `DefaultPlannedRuntimeParts` and, in `build_default_planned_runtime`, call `.with_hook_dispatcher_builder_factory(...)` on the production `RebornLoopDriverHostFactory` when it is present. `None` (the default) means no dispatcher is composed — behavior identical to the pre-hooks runtime (rollout-safety contract). The composition layer (`build_reborn_runtime`) resolves the flag via `HooksActivationConfig::from_env()` and builds the factory against this tenant's extension registry (per-tenant by construction — the function runs once per identity). Fail-closed: a malformed manifest hook fails the build here rather than composing a broken dispatcher. A per-run builder factory (not a captured dispatcher instance) is used so the host attaches a run-scoped milestone sink internally per build — per-run telemetry attribution, the nearai#3573 capture-and-stick lesson. All `DefaultPlannedRuntimeParts` construction sites (8 test sites across ironclaw_reborn, ironclaw_product_workflow, ironclaw_reborn_composition) updated with the new field defaulting to `None`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): e2e activation tests through build_default_planned_runtime (nearai#3934) Item 8 of nearai#3934. Add four end-to-end tests in crates/ironclaw_reborn/tests/loop_driver_host.rs that drive the *production* composition function `build_default_planned_runtime` with a per-run hook dispatcher builder factory shaped exactly like the composition layer's output (first-party builtin no-op observer + extension-declared `Installed`-tier hooks projected from a manifest entry through `HookRegistrar::install`), then build a host via the composed `host_factory` and invoke a capability: - flag OFF (no factory): allowed capability completes unaffected and reaches the inner host runtime port — the pre-hooks behavior / rollout-safety contract. - flag ON, first-party-only no-op observer: outcome unchanged, inner port reached — the builtin ships dark. - flag ON, extension-declared deny hook: capability denied through the composed runtime and the inner port is never reached (installed at the Installed tier via the registrar; OwnCapabilities scope keyed to the capability provider). - per-tenant isolation: tenant A's deny hook fires; tenant B (separate build_default_planned_runtime composition, no hooks) completes the same capability — proving no cross-tenant leakage. Security-audit-on-deny assertion is intentionally deferred: nearai#3922's SecurityAuditSink is not yet on reborn-integration. It lands with the audit-sink wiring follow-up. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): thread hook_dispatcher_builder_factory through shared reborn harness (nearai#3934) The root-crate `tests/support/reborn/harness.rs` constructs `DefaultPlannedRuntimeParts` directly; add the new `hook_dispatcher_builder_factory: None` field so the parity-test harness compiles. Default `None` keeps the harness on the no-hooks path (unchanged behavior). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks): proper error handling / safety annotations for activation production paths (nearai#3938 CI) The per-run dispatcher factory closure used `.expect()` on the first-party and extension hook installs, tripping the no-panics CI gate. These installs are pure replays of the install set already validated fail-closed (`?`) against a scratch builder at composition time, so they are genuine invariants. The factory type returns a non-Result `HookDispatcherBuilder` and is invoked deep in the run loop, so the documented `// safety:` suppression is the correct fix here. Hoisted the expect messages into `let` bindings so the `.expect(msg)` call fits on one line, keeping the scanner-required `// safety:` comment on the same line as the call after rustfmt. The malformed-manifest path (TOML projection) already uses real error propagation via map_err/`?` and is unaffected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): direct composition-loader coverage + activation-scope docs Address Codex non-blocking follow-ups on nearai#3938. Add three direct tests for the composition-layer hook loader (`install_extension_hooks` via `build_hook_dispatcher_builder_factory`), driving a real `ExtensionRegistry` with `[[hooks]]` declarations rather than mimicking the loader: - valid `own_capabilities` predicate hook installs at the Installed trust tier; the dispatcher carries the derived binding at BeforeCapability alongside the first-party no-op observer - malformed typed hook body (unknown `mode`) fails CLOSED with `RebornBuildError::InvalidConfig`, never a panic (the load-bearing degradation contract for untrusted external manifests) - a hook claiming `scope = same_tenant` without a verified grant is rejected by trust attenuation (fail-closed) No loader bug surfaced: `HookRegistrar::install` already returns `Result` on every malformed/over-scoped path and the loader maps it to `InvalidConfig` via `?`. Document activation scope at both the loader rustdoc and the `build_reborn_runtime` call site: production currently passes only `builtin_extension_registry()`, so third-party installed-extension hooks are not yet surfaced into the runtime path — only first-party-builtin and builtin-package-declared hooks activate today. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hooks): thread HooksActivationConfig through input; empty production catalog Two maintainability cleanups on nearai#3938 (firat review): Item 1 — config hygiene: stop reading HOOKS_ENABLED via std::env::var deep inside build_reborn_runtime. Add a typed `hooks: HooksActivationConfig` field to RebornRuntimeInput (default OFF) plus a `with_hooks_config` builder. The composition root now consumes the typed config; the env var is resolved ONCE at the edge (the reborn CLI's build_runtime_input) via HooksActivationConfig::from_env and threaded down. Testable without env mutation; matches the project's env → typed config → composition pattern. Item 2 — empty production first-party catalog: stop shipping NoOpObserverHook as a first-party builtin. install_first_party_hooks is now a no-op (empty catalog); the production type/install/export for a hook that does nothing is gone (removed from lib.rs exports). The activation machinery is still tested end-to-end through the real composition path via a new `build_hook_dispatcher_builder_factory_with` seam that takes a first-party installer; tests pass a `#[cfg(test)]` NoOpObserverHook through it. Pinned the empty-catalog-is-valid contract: flag ON + empty first-party set + no extension hooks composes a valid zero-binding dispatcher (not a panic/error). Reconciled the sibling loader tests/docs (f6c79c0): the in-module tests now drive the test-only seam; the reborn e2e tests already used a test-local no-op and are untouched. Updated activation-scope docs (loader rustdoc + build_reborn_runtime call site) to reflect the now-single live source (builtin-package-declared hooks). Deferred (not touched): switching to the canonical extension registry for third-party installed-extension hooks (nearai#3934 follow-on). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hooks): canonical registry + infallible plan + tenant-scoped counter docs/tests (nearai#3938) Addresses serrrfirat's thermo-nuclear re-review on 1e618d0. #1 (runtime.rs:839, canonical registry): make the extension registry a shared composition artifact. `build_local_dev` builds one `Arc<ExtensionRegistry>`, hands it to `HostRuntimeServices::new` AND stores it in `RebornLocalRuntimeServices.extension_registry`. Hook activation in `build_reborn_runtime` now consumes that same `Arc` instead of rebuilding a builtin-only sidecar, so capability dispatch and hook activation cannot drift. Third-party activation stays a follow-up, but it now follows the canonical registry rather than a separate path. #3 (hooks.rs factory machinery): replace the parse/validate/replay duplication + two prose-justified `.expect()` calls with a typed `HookInstallPlan`. TOML is projected once into typed entries, the full install set is validated once against a fresh builder (fail-closed via `?`), and `HookInstallPlan::rebuild` mints a fresh builder per run. The per-run path is infallible by construction: a plan only exists for an install set that already composed cleanly, so a deterministic replay from the identical fresh-empty start cannot fail. One extension-install code path (`project_extension_install_sets` + `install_extension_sets`) is shared by validation and rebuild. #4 (hooks.rs:295, predicate counter scoping): the evaluator/backend is intentionally tenant-scoped and shared across runs (rate/value caps keyed `(hook, tenant, capability)` with no run_id; a run-scoped limit would reset every run and enforce nothing). Document the split explicitly — per-run-fresh dispatcher, tenant-scoped predicate counters — in the module docs and fix the misleading "per-run isolation of hook state" wording in `ironclaw_reborn` (loop_driver_host.rs / runtime.rs). Add `predicate_counter_state_is_tenant_scoped_across_rebuilds`, which drives a real rate-cap predicate through two dispatchers from one factory and proves the second run sees the first run's recorded count. Rename `factory_mints_independent_dispatchers_per_call` -> `rebuild_mints_independent_dispatchers_per_call` and scope it to proving dispatcher freshness only. #6 (loop_driver_host tests): clarify that the hand-built builder factories cover host PLUMBING, not composition activation. Add `build_reborn_runtime_activates_hooks_through_real_composition_path`, which drives the real `build_reborn_runtime` with `HooksActivationConfig` threaded through `RebornRuntimeInput` (env-free) and the canonical registry, proving the production activation wiring composes. #2 (env boundary) and #5 (empty production catalog) were already fixed in 1e618d0; docs touched here for consistency. Known follow-up (not one of the six items, not introduced here): with the flag ON the standalone local-dev runtime does not yet reach `Completed` for a capability turn even with a zero-binding dispatcher — the composition root wires the dispatcher but not the companion hooked-prompt dependencies. The new runtime test asserts `is_terminal()` + the capability path and documents the gap. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(hooks): third-party hook-only projection core (flag, newtype, quarantine, caps) Steps 1-6 of third-party extension hook activation via hook-only projection: - Step 1: HOOKS_THIRD_PARTY_ENABLED sub-flag on HooksActivationConfig (default OFF; is_third_party_enabled() requires master flag too). Resolved at the CLI edge via from_env(). - Step 2: tenant_extension_root(&TenantId) derives the fixed /system/extensions/<tenant> root from identity (never caller-supplied); projection-layer strict-child / no-`..` containment check. - Step 3: build_hook_projection_registry assembles a HookProjectionRegistry (type-enforced hook-only newtype: no Deref / conversion back to ExtensionRegistry, so it can never reach HostRuntimeServices::new / the capability path). Sub-flag OFF => builtin-only, byte-identical to nearai#3938. - Step 4/4a: atomic per-extension quarantine — untrusted (InstalledLocal) sets validated whole against a scratch builder, committed only if the whole set passes; any failure drops the extension's hooks entirely, emits a hook.quarantined security_audit tracing event (warn!, not info!), and continues. Trusted (HostBundled) sources stay fail-closed-whole-build. - Step 5: MAX_INSTALLED_EXTENSIONS_CONSIDERED / MAX_TOTAL_HOOKS_PER_TENANT DoS caps; count_total_bindings() accessor on HookDispatcher(Builder); pre-read MAX_MANIFEST_BYTES bound via read_file_bounded in discovery. - Step 6: third-party WASM stays out (loader registrar has no wasm_runtime) => WASM-bodied hook quarantines + build continues. Registrar-only invariant: projection installs go exclusively through HookRegistrar::install (ceiling + spoof-blocked owning_extension), never the direct builder installer API. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(hooks): FS-scoped tenant isolation (Option 1), trust matrix, registrar-only assertion Resolve the discovery/path conflict: the discovery layer hardcodes package roots to /system/extensions/<id> because the per-tenant RootFilesystem is the scope boundary (as with every other tenant-scoped resource), not a tenant path segment. So: - tenant_extension_root -> fixed /system/extensions (no tenant segment). The per-tenant RootFilesystem handed to discovery IS the isolation boundary. Documented as load-bearing; the openat2(RESOLVE_BENEATH)/O_NOFOLLOW backend hardening follow-up is what protects it (gating note kept prominent). - build_local_dev mounts /system/extensions to a per-owner host subtree under the storage root (per-identity by construction, not a process-global mount); exposed via RebornLocalRuntimeServices.extension_filesystem. - enforce_root_containment retained as defense-in-depth. Tests: - Integration (real build_hook_projection_registry + build_hook_dispatcher_ builder_factory through a fake RootFilesystem, not a loader look-alike): containment (hook present / capability absent by construction), FS-as-boundary tenant isolation proof (two distinct per-tenant filesystems; A can't see B), bad dir name skipped, id mismatch not a panic, surplus-extensions DoS cap, sub-flag OFF discovers nothing. - Per-hook-point trust matrix: BeforeCapability installed deny IS allowed and fires (Gate reachable); before_prompt predicate quarantined + build continues; after_model/after_capability/after_checkpoint/event_triggered WASM-only => quarantined + build continues; owning_extension derived (not spoofable). - Discovery pre-read bound: oversized manifest rejected via stat WITHOUT reading the body (fake fs panics on get); within-bound proceeds to read. - ironclaw_architecture source assertion: the hooks.rs projection path never calls install_installed_* directly (registrar-only invariant). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(hooks): correct Option-1 path-shape references in test comments Update the third-party projection integration-test module docs to reflect the FS-scoped isolation model (fixed /system/extensions root; per-tenant filesystem is the boundary), not the abandoned /system/extensions/<tenant> path segment. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks): tolerant+bounded third-party discovery, structural hook-only containment Addresses Codex P1/P2 + serrrfirat P1 on nearai#3951. Critical 1 (discovery-stage DoS): add `ExtensionDiscovery::discover_with_manifest_contracts_tolerant_bounded` (+ `discover_extensions_tolerant_bounded` host-runtime wrapper). It lists+sorts the root once, then reads/parses at most `max_extensions` manifests, recording the surplus as quarantines WITHOUT reading them. The hook projection calls this with `MAX_INSTALLED_EXTENSIONS_CONSIDERED`, so the count cap fires before the per-manifest read storm. New all-or-nothing path delegates to a shared `load_package_entry` so per-package semantics are identical. Critical 2 (fail-open): tolerant discovery quarantines a single malformed/oversized/id-mismatched package and CONTINUES; valid siblings still load. The builtin-only fallback is now reserved solely for failure to LIST THE ROOT (directory unreadable). One bad manifest can no longer drop a tenant's entire legitimate third-party hook set. Refinement 3: the per-tenant hook budget is consumed only AFTER a successful merge, so a quarantined/duplicate package no longer burns budget. Refinement 4: the registrar-only arch assertion now scans the WHOLE composition crate (every non-test source) and forbids all installed-tier-minting primitives crate-wide (`install_installed_*`, `install_observer(`, `insert_binding(`, `HookTrustClass::Installed`) — not just a hooks.rs substring scan. Installed-tier bindings can only be minted via `HookRegistrar::install`. serrrfirat P1 (structural containment): `HookProjectionRegistry` no longer wraps `ExtensionRegistry`. It carries `Vec<HookProjection>` — hook metadata only (id/version/source/root/[[hooks]]). The projection literally cannot reach capabilities because it does not hold them; containment is by data shape, not a withheld conversion. Removes the `ExtensionPackageView` ceremony. Tests: bounded read-storm cap (read-counting fs panics on surplus), tolerant per-package quarantine, root-unreadable fallback, quarantined-package-does-not- consume-budget, malformed-sibling-survives at the projection layer. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hooks): address serrrfirat review — tenant-attributed install audits + hooks decomposition Addresses the maintainability review on nearai#3951. Findings #1 (narrow hook-only boundary), #2 (per-extension discovery quarantine + mixed-batch test), and #5 (behavioral arch-test invariant) were already satisfied by the head commit (2b62597); this commit closes the two remaining items and hardens the arch test against the decomposition: - #3 (tenant attribution): add `build_hook_dispatcher_builder_factory_for_tenant`, threading the authenticated `tenant_id` (and its derived extension root) into the install-time quarantine-audit seam. `build_reborn_runtime` now calls it, so install-time quarantine audits carry the real tenant instead of the synthetic `reborn-hook-projection` fallback (closing the split where only discovery-time audits were attributed). New caller-driven test `for_tenant_entry_point_attributes_install_time_quarantine_to_real_tenant` asserts attribution via a deterministic thread-local audit capture (immune to tracing's process-wide max-level filter under parallel tests). - #4 (decomposition): split the 1.7k-line `hooks.rs` into a focused `hooks/` module — `mod.rs` (flag/config + public surface), `projection.rs` (hook-only `HookProjection`/`HookProjectionRegistry` containment + discovery/admission), `factory.rs` (first-party install, per-extension quarantine validation, fresh-per-build replay), `audit.rs` (`hook.quarantined` emission), and `tests.rs` (the test matrix). Behavior-preserving; no logic change. - arch test: skip dedicated test-module files in the registrar-only scan so the #4 decomposition cannot break it; the whole-crate behavioral invariant is preserved. - audit emission uses `debug!` (not `warn!`) per the background/hook-path logging rule, on the stable filterable `security_audit` target. - gemini nearai#353: add the documented no-empty-segment guard to `enforce_root_containment` (defense-in-depth, not relying on VirtualPath canonicalization). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(hooks): cover hook-entry rejection branches; document InvocationCount cap semantics (nearai#3938) Address henrypark133 review (review 4367870023): - Add extension-manifest tests for the three previously-uncovered hook-entry validation branches: non-table `[[hooks]]` element, whitespace-only `id`, and oversized entry (HookEntryTooLarge). - Document the InvocationCount inclusive-allow / deny-on-overflow semantics inline at the comparison site; behavior unchanged and still pinned by the cap test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(deps): pin kuchikikiki to 0.9.1 (0.9.2 yanked) cargo-deny failed on the yanked kuchikikiki 0.9.2 pulled in transitively via readabilityrs. Downgrade to 0.9.1 at the lockfile level. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(reborn-cli): assert hooks config threaded in caller test (nearai#3938) Addresses the review finding that `build_runtime_input_maps_configured_cli_identity` exercised `build_runtime_input` but never asserted the `hooks` field, so a regression silently dropping `with_hooks_config(HooksActivationConfig::from_env())` or flipping the default-OFF rollout-safety contract would pass. Per `.claude/rules/testing.md` ("Test Through the Caller"): add two assertions to the existing caller-level test: - threading: `runtime_input.hooks == HooksActivationConfig::from_env()`, proving the env-resolved config is actually threaded through and not dropped. Verified via TDD that dropping the wiring fails this assertion (run under HOOKS_ENABLED=1). - default-OFF: when `HOOKS_ENABLED` is unset, `!runtime_input.hooks.is_enabled()`, guarded to skip if the CI environment exports the flag so it only pins the contract it claims to. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(hooks): cover build_reborn_runtime third-party wiring + async dir create (nearai#3951) Address serrrfirat review findings M1 and L2. M1: add an integration test in tests/runtime.rs that drives build_reborn_runtime with HooksActivationConfig::enabled().with_third_party_enabled(true), a real /system/extensions manifest tree on the local-dev host filesystem, and tenant attribution. Asserts the runtime builds, starts a conversation turn, and shuts down cleanly — exercising the runtime.rs third-party discovery input + projection registry + tenant-threading wiring that was previously uncovered (the projection tests call build_hook_projection_registry / the dispatcher factory directly, and every other build_reborn_runtime call used the default disabled config). Verified the test fails when the wiring is broken. L2: switch the new factory.rs blocking std::fs::create_dir_all for the extensions host root to tokio::fs::create_dir_all(...).await with the same error mapping, so it no longer blocks the tokio executor thread inside the async build_local_dev. The two pre-existing std::fs calls (lines 132/136) are out of this PR's diff per the posted promise and are left untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(hooks): L1 quarantine-surfacing gate doc, L3 robust test-mod strip, M1 coverage-gap TODO Address serrrfirat 2026-06-03 review (M1/L2 already landed in 9866793). L1 (security observability): hook.quarantined audit events are emitted only via tracing at the security_audit target / debug! level, which production typically disables. Document durable quarantine surfacing as a hard production-enablement prerequisite for HOOKS_THIRD_PARTY_ENABLED, alongside the existing openat2(RESOLVE_BENEATH)/O_NOFOLLOW FS-hardening note, at all three gate doc sites: HooksActivationConfig (hooks/mod.rs), the runtime.rs composition -root gate comment, and the audit.rs module doc. L3 (robustness): strip_test_module matched #[cfg(test)]\nmod tests specifically and only the first occurrence. Generalize the anchor to #[cfg(test)]\nmod (any module name) so a refactor that renames the test module or adds a second #[cfg(test)] mod block is still fully stripped, preventing false positives in the FORBIDDEN_INSTALLED_PRIMITIVES architecture scan. M1 (test coverage): the build_reborn_runtime third-party wiring test already landed in tests/runtime.rs (9866793). Add the reviewer-requested TODO preserving the removed test's Cancelled-outcome coverage gap: the stub local-dev gateway cancels the turn before any capability dispatches, so the test exercises discovery + projection + tenant-threading at build/start but not end-to-end hook enforcement. NOTE: third-party discovery is intentionally tolerant (skips unparseable manifests), so this test catches compile-time field/arg regressions and build-path failures but not a silent manifest-read drop; documented for the reviewer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(hooks): correct in-memory backend warning text; delegate test ctor (nearai#3938) Address serrrfirat review (2026-06-03): - Low: the in-memory backend warning claimed the LRU cap is shared across tenants, but the Reborn composition constructs a fresh InMemoryPredicateStateBackend per tenant. Rewrite the warn! text and doc comment so the real limitation (process-local replay dedup for multi-host deployments) is accurate, and note the backend is per-tenant in this composition. - Nit: PredicateEvaluator::with_backend (test-only) and with_state_backend had identical bodies; delegate with_backend to with_state_backend so they stay in lockstep. The Medium finding (hooks_config assertion in build_runtime_input caller test) was already addressed in 218a1de. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
14 tasks
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
3 participants
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.
Summary
Complete v1 of the Reborn loop hooks framework per #3524. Lands the
trust primitives, sealed decision types, dispatcher, port middleware,
predicate evaluator, telemetry, snippet injection, integration tests,
and the Reborn composition seam — plus a four-finding security audit,
an eight-item follow-up batch (FU1–FU8), and a design-time validation
pass (prior-art comparison, STRIDE threat model, real-hooks ergonomics
exercise, operator runbook) — all in this PR.
Design backstory: #3524 (comment)
Design-time validation artifacts
Four companion docs in
crates/ironclaw_hooks/docs/:prior-art.mdwhydivergences.threat-model.mdreal-hooks-findings.mdoperator-runbook.mdWhat's demonstrable end-to-end
An extension ships a
[[hooks]]manifest entry.HookRegistrarconstructs a
PredicateBackedBeforeCapabilityHook, registers it viatier-specific installer, and the registrar wires the dispatcher
through
HookDispatcherBuilder::build_arc().RebornLoopDriverHostFactory::with_hook_dispatcher_factory(...)plugsin a per-run dispatcher factory. From there:
polymarket.place_orderinvocations 1–10 in a 24h window: forwarded to inner port.HookedLoopCapabilityPortintercepts; tenant-keyed sliding-window counter trips; outcome isCapabilityOutcome::Denied { reason_kind: hook_predicate_denied }.PauseApproval-emitting hook surfaces asCapabilityOutcome::ApprovalRequired { gate_ref: gate:hook-approval-<uuid>, .. }— gate-ref is a v4 UUID (122 random bits per RFC 4122, regression-tested against 20k draws for collision-freedom).ThreadBackedCapabilityInputResolverto extract the staked value from invocation args; rolling sum tracked through the predicate evaluator's bounded (MAX_HISTORY_KEYS = 8192, LRU-evicting) state map."Untrusted hook content: <body>"as a system message in the prompt bundle, subject to a 4 KiB aggregate budget.HookDispatched,HookDecisionEmitted,HookFailed) flow into the host'sLoopHostMilestoneSinkand project intoRuntimeEvent::Hook{Dispatched,DecisionEmitted,Failed}for durable audit.Global/OwnCapabilities/SameTenant) enforced at dispatch — anown_capabilities-scoped hook only fires for its declaring extension's providers.MAX_HOOKS_PER_EXTENSION = 32,MAX_HOOKS_PER_EXTENSION_PER_KIND = 8) reject hook-registration flood attempts at the registrar boundary, pre-flight.crates/ironclaw_reborn/tests/hooks_integration.rs(13 scenarios) andcrates/ironclaw_hooks/tests/real_hooks.rs(6 scenarios).What this PR ships
New crates
crates/ironclaw_hooks/— hook framework with:HookTrustClass { Builtin, Trusted, Installed, SelfAuthored }) with type-enforced impl pairingHookId= blake3 of length-prefixed fields;to_hex()format pinned as cross-crate contract)BeforeCapabilityHookDecision,HookPatch,ObserverFact)install_builtin_*/install_trusted_*/install_installed_*) and observer variantsHookDispatcherBuilderwith terminalbuild_arc()— the only public construction pathPredicateandWasmbody variants;#[non_exhaustive]+ builder API; install-time validation including window parsing, scope-vs-grant checks, and per-extension capsMAX_HISTORY_KEYS = 8192withevictions_observed()operator metricGate=FailClosed,Observer=FailIsolated) +catch_unwindper hook +tokio::time::timeout+ same-dispatch poison re-checkUuidHookGateRefFactoryminting v4 UUID gate-refs (unguessability pinned by collision-freedom test across 20k draws)ThreadBackedCapabilityInputResolver+HookCapabilityInputResolverAdapterSelfAuthoredHookSink(monotonic-restriction only — no Allow, no Effect; run-scoped only)HookMilestoneSinkadapter pattern preservingironclaw_turnsboundarytest-supportfeature flag exposingSanitizedArguments::for_tests(value)for external hook-author TDDcrates/ironclaw_prompt_envelope/— new leaf crate (no ironclaw deps);wrap_untrusted(source, trust, body)with closed-vocabularyEnvelopeSource, 4 KiB cap, instruction-marker denylist.memory_context.rsmigrated to delegate here.Reborn composition seam
crates/ironclaw_reborn/src/loop_driver_host.rswith_hook_dispatcher_factory(|| -> Arc<HookDispatcher>)— per-run dispatcher seam for clean isolation across resume/replay (FU8)with_hook_dispatcher(Arc<HookDispatcher>)— legacy back-compat adapterwith_capability_input_resolver(...)— production NumericSum wiring (FU5)crates/ironclaw_reborn/src/milestone_events.rs— projectsLoopHostMilestoneKind::Hook*intoRuntimeEvent::Hook{Dispatched,DecisionEmitted,Failed}for durable audit (FU3).crates/ironclaw_reborn/tests/hooks_integration.rs— 13 end-to-end scenarios including predicate deny, PauseApproval gate-ref minting, milestone projection, observer middleware (model/capability/checkpoint/panic-isolation), NumericSum against real inputs, legacy vs per-build dispatcher isolation, and manifest-scope filtering.Turns & events crate additions
LoopHostMilestoneKind::{HookDispatched, HookDecisionEmitted, HookFailed}(closed-vocabularyHookDecisionSummary)HookMilestoneSinktrait +InMemoryHookMilestoneSinktest helper +RunScopedHookMilestoneSinkadapter (ironclaw_turnsside, no ironclaw_hooks dep)HookDispatched+ one terminatorRuntimeEvent::{HookDispatched, HookDecisionEmitted, HookFailed}— durable audit substrateArchitecture boundary
ironclaw_turns→ironclaw_hooksforbidden-deps rule (boundary test enforces it)BoundaryRuleforironclaw_hooks(forbidshost_runtime/dispatcher/secrets/network/wasm/reborn)ironclaw_prompt_envelopeis a leaf crateTrust-property compile-time enforcement
The load-bearing claim — an Installed-tier hook cannot mint
Allow— is enforced at the type level via two mechanisms:RestrictedGateSinktrait has noallowmethod.BeforeCapabilityHookImpl::{Privileged, Restricted}variants arepub(crate). The only public path toPrivilegedis viainstall_builtin_*/install_trusted_*, which always construct Builtin/Trusted bindings.Regression tests:
dispatch::tests::compile_time_seal_test,installed_binding_cannot_be_paired_with_privileged_impl. Same sealing pattern onRestrictedMutatorSink,HookPatch,ObserverFact,SelfAuthoredHookSink.Security audit findings — all closed
Installed + PrivilegedpairingHistoryKey+ per-build dispatcher (FU8)BeforeCapabilityHookContext.provider+ dispatcher scope filterparse_windowpanic on non-ASCIIThreat-model gaps — all High & Med closed
STRIDE pass across 7 adversary classes surfaced ~35 attack vectors. All High and Med gaps closed in this PR:
MAX_HOOKS_PER_EXTENSION = 32pre-flight capMAX_HOOKS_PER_EXTENSION_PER_KIND = 8SanitizedArgumentspublic API (onlyextract_numericby named path); reassess at Installed-WASMMAX_HISTORY_KEYS = 8192per map + LRU eviction +evictions_observed()metricoperator-runbook.md§1Real-hooks ergonomics findings — all closed
Three hooks built from outside the crate (rate-cap predicate, NumericSum approval-gate, Trusted PII-redaction mutator) surfaced 7 friction findings; all closed in this PR:
unresolved()blocked external dispatch testspub fn unresolved()OnExceededAction+GateDecisionView::Denytest-supportfeature +SanitizedArguments::for_testsExtensionIdtypes confused manualHookId::deriveFrom<&ironclaw_host_api::ExtensionId>+ cross-link rustdocHookManifestEntrystruct-literal fragility#[non_exhaustive]+new(id, kind, body)+with_*builderHookPriorityrustdoc with when-to-deviate guidance, named constantsFollow-up batch FU1–FU8 — all merged
HookIdformat pin testRuntimeEventprojection for hook milestonesRuntimeEvent::Hook*+milestone_events.rsprojectionCapabilityInputResolverfor NumericSumThreadBackedCapabilityInputResolverwired through factoryHookDispatcherBuilderfor type-enforced wiringHookDispatcher::newispub(crate); builder is the only public pathArc<dyn Fn() -> Arc<HookDispatcher>>What this PR deliberately does NOT ship
Wasmbody type; runtime execution requireswasmtimeintegration. Will need its own threat model.Test plan
cargo test -p ironclaw_hooks --all-features— 151 unit + 1 + 6 integration tests passcargo test -p ironclaw_reborn— 56 unit + 13 hooks_integration + 5 other passcargo test -p ironclaw_prompt_envelope— leaf crate tests passcargo test -p ironclaw_turns— including L3 snapshots + L4 pairing matrixcargo test -p ironclaw_events— newRuntimeEvent::Hook*variantscargo test -p ironclaw_architecture— new boundary rules enforced (turns ↛ hooks, hooks ↛ host_runtime/secrets/network/wasm/reborn)cargo clippy --workspace --all-targets -- -D warnings— cleancargo fmt --all -- --check— cleanDiff stats
59 files changed, ~14,949 insertions, 73 deletions vs
reborn-integrationbase. 41 commits.🤖 Generated with Claude Code