feat(hooks): activate hook framework in production behind HOOKS_ENABLED flag (#3934) - #3938
Conversation
…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>
… 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>
…ed_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>
…me (#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>
…born 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>
github-actions
Bot
added
scope: dependencies
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the production activation of the hook framework, enabling declarative hook entries in extension manifests and ensuring per-run isolation via a new dispatcher factory. Key changes include structural validation for hooks in the manifest parser, a feature-flagged activation path in the composition layer, and end-to-end tests for tenant isolation. Review feedback identified a bug where the hook initialization incorrectly ignored non-builtin extensions and recommended refactoring redundant TOML parsing while ensuring deterministic extension ordering.
| // build here rather than silently composing a broken dispatcher. | ||
| let hooks_config = HooksActivationConfig::from_env(); | ||
| let hook_dispatcher_builder_factory = { | ||
| let registry = builtin_extension_registry()?; |
There was a problem hiding this comment.
The hook framework is being initialized using only the built-in extension registry, which means hooks declared in discovered or user-installed extensions will be ignored. Since services.extension_registry already contains the complete set of extensions (including built-ins), it should be used here instead.
let registry = &services.extension_registry;There was a problem hiding this comment.
Resolved in ce0c127. Hook activation no longer builds against a fresh `builtin_extension_registry()`. `build_local_dev` (factory.rs) now constructs ONE `Arc` and hands the same `Arc` to both `HostRuntimeServices::new` (capability dispatch) and `RebornLocalRuntimeServices.extension_registry`; `build_reborn_runtime` consumes that exact `Arc` via `Arc::clone(&local_runtime.extension_registry)`. The moment installed/discovered packages are inserted into that canonical registry upstream, the loader picks them up here with no call-site change.
| for package in registry.extensions() { | ||
| let manifest = &package.manifest; | ||
| if manifest.hooks.is_empty() { | ||
| continue; | ||
| } | ||
| let mut entries = Vec::with_capacity(manifest.hooks.len()); | ||
| for hook in &manifest.hooks { | ||
| // Already validated above; this parse cannot fail, but we surface | ||
| // any error fail-closed rather than unwrapping. | ||
| let entry: HookManifestEntry = toml::from_str(&hook.raw_toml).map_err(|error| { | ||
| RebornBuildError::InvalidConfig { | ||
| reason: format!( | ||
| "extension `{}` hook `{}` failed re-projection: {error}", | ||
| manifest.id.as_str(), | ||
| hook.local_id | ||
| ), | ||
| } | ||
| })?; | ||
| entries.push(entry); | ||
| } | ||
| extension_install_sets.push((manifest.id.clone(), manifest.version.clone(), entries)); | ||
| } |
There was a problem hiding this comment.
This loop duplicates the iteration and TOML parsing logic already performed inside install_extension_hooks. Consider refactoring install_extension_hooks to return the projected entries to avoid redundant work and improve code clarity. When populating extension_install_sets, use ironclaw::extensions::naming::normalize_extension_names instead of a HashSet to preserve deterministic ordering and ensure validation. Note that while reducing redundant work is beneficial, code clarity and correctness should be prioritized over micro-optimizations in non-performance-critical paths.
References
- Prioritize code clarity and correctness over micro-optimizations in non-performance-critical paths where the number of elements is small or bounded.
- Use ironclaw::extensions::naming::normalize_extension_names instead of converting extension name collections to HashSet and back to preserve deterministic ordering and ensure validation.
- Path traversal characters (/, , .., \0) in extension names must be validated at the public API boundary for all operations (install, activate, remove) to prevent security vulnerabilities.
There was a problem hiding this comment.
Addressed in ce0c127. The duplicated iterate-and-parse path is gone: there is now a single `HookInstallPlan` that projects every extension's `[[hooks]]` TOML into typed `HookManifestEntry` values exactly ONCE (`project_extension_install_sets`), validates the full set against a scratch builder once, and replays the typed (already-parsed) entries per run via a shared `install_extension_sets`. No second TOML reparse, no scratch-builder-plus-replay divergence. Extension ordering is the registry's deterministic `extensions()` order, not a HashSet round-trip.
![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: b82e035ad3
ℹ️ 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".
| // build here rather than silently composing a broken dispatcher. | ||
| let hooks_config = HooksActivationConfig::from_env(); | ||
| let hook_dispatcher_builder_factory = { | ||
| let registry = builtin_extension_registry()?; |
There was a problem hiding this comment.
When HOOKS_ENABLED is on, this code always builds the hook dispatcher from builtin_extension_registry(), which only contains the built-in package. As a result, hooks declared in installed extension manifests are never projected/installed, so extension policy hooks silently do not run in production even though the feature flag is enabled. The dispatcher should be built from the same per-tenant extension registry the runtime is actually serving, not a freshly created built-ins-only registry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ce0c127. The dispatcher is no longer built from a builtins-only `builtin_extension_registry()`. It is built from the same per-tenant `Arc` the runtime serves capability dispatch through (shared `Arc` from `build_local_dev`, threaded via `RebornLocalRuntimeServices.extension_registry`). So once installed extension packages are inserted into that canonical registry upstream, their declared hooks are projected/installed here automatically — no separate wiring path.
|
@henrypark133 @serrrfirat flagship review please — this activates the hook framework in production (issue #3934) behind |
Codex review (advisory)Verdict: APPROVE with the no-panics annotations + scope/test follow-ups. No critical issues — default-OFF flag path is safe (no dispatcher composed unless config produces a factory), and per-tenant isolation is structurally sound (registry/evaluator/factory built inside per-identity Recommendations
These align with the carry-forwards already in the PR body. Verdict stands at APPROVE once the no-panics annotations land. |
… 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>
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Thermo-nuclear code-quality pass: I would not approve this shape as-is. The main issue is structural, not local correctness: hook activation is being composed as a parallel sidecar instead of using the runtime's canonical config and extension-registry boundary. That makes the feature harder to reason about and leaves the tests proving a reimplementation rather than the production path.
| // build here rather than silently composing a broken dispatcher. | ||
| let hooks_config = HooksActivationConfig::from_env(); | ||
| let hook_dispatcher_builder_factory = { | ||
| let registry = builtin_extension_registry()?; |
There was a problem hiding this comment.
This is the core structural problem in the PR: hook activation builds against a fresh builtin_extension_registry() instead of the registry the runtime actually uses. The host runtime is composed with its own registry in factory.rs, so hooks become a sidecar reconstruction rather than a natural extension of capability dispatch. The comment says this is "this tenant's extension registry," but the code is built-in-only and independent. Can we push hook activation to consume the same canonical extension registry/source that HostRuntimeServices uses, rather than rebuilding one here?
There was a problem hiding this comment.
Resolved in ce0c127 (see the threaded reply on the later runtime.rs:839 comment for the full detail). The extension registry is now a shared composition artifact: `build_local_dev` builds one `Arc` and hands it to BOTH `HostRuntimeServices::new` and the hook factory (`Arc::clone(&local_runtime.extension_registry)`). No more `builtin_extension_registry()` rebuild at the call site — capability dispatch and hook activation cannot drift.
| // registry (per-tenant scoping by construction: this whole function runs | ||
| // once per identity). Fail-closed: a malformed manifest hook fails the | ||
| // build here rather than silently composing a broken dispatcher. | ||
| let hooks_config = HooksActivationConfig::from_env(); |
There was a problem hiding this comment.
This bypasses the runtime input/config boundary. RebornRuntimeInput explicitly says the CLI builds runtime inputs from env/config, but build_reborn_runtime now reaches into process env itself. That makes hook activation a hidden global side effect rather than a per-runtime/per-tenant configuration value. Please thread HooksActivationConfig through RebornRuntimeInput or RebornBuildInput, and keep env parsing at the CLI/config edge.
There was a problem hiding this comment.
Fixed in 1e618d0. build_reborn_runtime no longer reads HOOKS_ENABLED itself. Added a typed hooks: HooksActivationConfig field to RebornRuntimeInput (default OFF) plus a with_hooks_config builder. The env var is now resolved ONCE at the edge — the reborn CLI's build_runtime_input calls HooksActivationConfig::from_env() and threads the typed value down — and the composition root just consumes it. This restores the input/config boundary and makes the flag testable without env mutation.
| } | ||
|
|
||
| let evaluator_for_factory = Arc::clone(&evaluator); | ||
| let factory: HookDispatcherBuilderFactory = Arc::new(move || { |
There was a problem hiding this comment.
This factory has too much incidental machinery: validate into a scratch builder, reparse the TOML into extension_install_sets, then replay installs with production .expect() calls. I think there's a code-judo move here: parse once into a typed HookInstallPlan, validate that plan once, and have the plan build a fresh builder per run. Then the closure is genuinely infallible by construction instead of relying on comments plus expect.
There was a problem hiding this comment.
Addressed in ce0c127. Replaced the validate-into-scratch-builder / reparse-TOML-into-install-sets / replay-with-`.expect()` machinery with a typed `HookInstallPlan`: parse + validate ONCE (the only fallible step, fail-closed via `?`), then `rebuild()` mints a fresh builder per run by replaying the already-typed entries from the identical fresh-empty start. The closure is infallible by construction — the only residual is an `unreachable!` guarding the type-carried invariant (a plan only exists for an install set that already composed cleanly), not a per-run `.expect()`.
| /// are identified by a stable canonical path, not a content-addressed | ||
| /// extension id. They are installed regardless of which extensions are | ||
| /// present. | ||
| fn install_first_party_hooks( |
There was a problem hiding this comment.
This no-op production builtin feels like scaffolding that escaped into the real catalog. It adds a stable builtin identity, public type, install path, and behavior assertions for a hook whose product behavior is intentionally nothing. Can we keep the first-party catalog empty until there is a real builtin hook, and test the activation machinery with test-only hooks or extension-declared hooks instead?
There was a problem hiding this comment.
Fixed in 1e618d0. Removed NoOpObserverHook from the production first-party catalog: install_first_party_hooks is now a no-op (empty catalog), and the public type/export is gone (dropped from lib.rs). No production builtin identity, install path, or behavior assertions ship for a hook that does nothing.
The activation machinery is still tested end-to-end through the real composition path: I added a build_hook_dispatcher_builder_factory_with seam that takes a first-party installer, and the in-module tests pass a #[cfg(test)] NoOpObserverHook through it. I also 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).
The sibling loader tests/docs from f6c79c0 were reconciled rather than reverted: the in-module tests now drive the test-only seam, and the reborn e2e tests already used a test-local no-op so they're untouched. Activation-scope docs updated to reflect the now-single live source (builtin-package-declared hooks).
| assert!(runtime.invocations().is_empty()); | ||
| } | ||
|
|
||
| // ─── Hook framework activation (#3934) e2e through build_default_planned_runtime ── |
There was a problem hiding this comment.
These tests reimplement the composition loader instead of driving it. first_party_only_hook_factory and extension_deny_hook_factory duplicate the shape of ironclaw_reborn_composition::hooks, so they can pass even if the real production activation is wired to the wrong registry or config boundary. Please move the activation coverage into a focused composition/runtime test that exercises build_hook_dispatcher_builder_factory and build_reborn_runtime through the actual composition path.
There was a problem hiding this comment.
Addressed in ce0c127. The activation coverage now drives the REAL composition path: `predicate_counter_state_is_tenant_scoped_across_rebuilds`, `rebuild_mints_independent_dispatchers_per_call`, `enabled_config_with_empty_production_catalog_yields_valid_zero_binding_factory`, the malformed/wider-scope fail-closed tests (all in `ironclaw_reborn_composition::hooks`), and `build_reborn_runtime_activates_hooks_through_real_composition_path` (in `ironclaw_reborn_composition::runtime`) exercise `build_hook_dispatcher_builder_factory` + `build_reborn_runtime` directly. The hand-built factories in `loop_driver_host.rs` are explicitly re-scoped (see the new NOTE there) to host-factory PLUMBING doubles only — not activation coverage — so they can no longer masquerade as proof the production wiring is correct.
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>
|
Addressed the two non-blocking Codex follow-ups (APPROVE verdict; quality items, not blockers) in f6c79c0: 1. Direct composition-loader test coverage. Added three tests in
No loader bug surfaced: 2. Activation-scope doc clarity. Added a code-level note at both the Verification (all clean): |
…oduction 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>
|
@serrrfirat re-review please — addressed your two maintainability findings (SHA 1e618d0):
Deferred (your other two notes): the |
|
@serrrfirat @henrypark133 friendly ping — every item from your review is addressed as of |
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Thermo-nuclear re-review on 1e618d0: I still would not approve this shape. The env-boundary and no-op-production-hook issues were fixed, and the focused composition hook tests pass locally. The remaining blocker is structural: the new factory claims per-run isolation, but the stateful predicate evaluator/backend is captured once per runtime and reused by every per-run dispatcher. That contradicts the existing host-factory contract and leaves readers unable to tell whether hook predicate state is supposed to be run-scoped or tenant-scoped. Please make that state boundary explicit in the design and tests before shipping.
| // Postgres/libSQL backend (#3933) drops in here without touching the rest | ||
| // of the wiring. | ||
| let backend: Arc<dyn PredicateStateBackend> = Arc::new(InMemoryPredicateStateBackend::new()); | ||
| let evaluator = Arc::new(PredicateEvaluator::with_state_backend(Arc::clone(&backend))); |
There was a problem hiding this comment.
This creates one PredicateEvaluator / in-memory backend outside the per-run factory, then every dispatcher minted by factory() installs predicate hooks with clones of that same evaluator. That undercuts the per-run isolation story: RebornLoopDriverHostFactory says the builder-factory path localizes predicate counters to one host/run, and PredicateBackedBeforeCapabilityHook stores the evaluator Arc, while rate/value-cap keys are only hook/tenant/capability. So two host builds from the same runtime can share stateful predicate counters even though the dispatcher itself is fresh. If counters are meant to be run-scoped, instantiate the evaluator/backend inside the factory closure. If they are intentionally tenant-scoped, the host-factory/evaluator docs and tests need to say that explicitly, and the factory_mints_independent_dispatchers_per_call test should be replaced or supplemented with a stateful predicate test that proves the intended boundary.
There was a problem hiding this comment.
Resolved in ce0c127 with the explicit decision you asked for: the predicate counters are INTENTIONALLY tenant-scoped, and that boundary is now documented and tested rather than left ambiguous.
Why tenant-scoped (not run-scoped): rate/value-cap counters are keyed (hook, tenant, capability) with no run_id. A run-scoped rate limit would reset to zero at the start of every run and could never enforce a cross-run budget — useless. So the evaluator/backend is deliberately built once per tenant and shared across runs.
What changed:
- (a) Made it explicit in docs. New module section "Predicate counter scoping (TENANT-scoped, deliberately shared across runs)" spells out the split: the dispatcher is per-run-fresh (slot-poisoning, registry edits do not leak), but the predicate counter STATE is tenant-scoped and shared across runs by design. The evaluator is captured once in the new
HookInstallPlanand reused by everyrebuild. - (b) Fixed the misleading wording. The "per-run isolation of hook state" / "predicate counters ... do not leak into the next run" comments in
ironclaw_reborn(loop_driver_host.rsdoc onhook_dispatcher_factory+with_hook_dispatcher, the per-build mint comment, andruntime.rs) now say dispatcher-local state is per-run while predicate counters are tenant-scoped and shared. - (c) Added the stateful test.
predicate_counter_state_is_tenant_scoped_across_rebuildsdrives a realInvocationCount { max = 1 }rate-cap predicate through the actual composition path: build the factory once, mint TWO dispatchers, dispatchbefore_capabilitythrough the first (allowed, count -> 1), then through the second freshly-rebuilt dispatcher (DENIED, because it observes the count the first run recorded). If the evaluator were per-run, the second would reset to zero and allow — the assertion fails.factory_mints_independent_dispatchers_per_callis renamed torebuild_mints_independent_dispatchers_per_calland scoped to proving dispatcher freshness only.
The dispatcher stays per-run-fresh; only the predicate counter state is tenant-scoped, and the two halves are pinned by separate tests.
| // hooks; this call site simply doesn't feed it a third-party registry | ||
| // yet. Wiring the per-tenant installed-extension registry here is the | ||
| // follow-up that turns on live third-party activation. | ||
| let registry = builtin_extension_registry()?; |
There was a problem hiding this comment.
This still composes hooks from a freshly rebuilt builtin_extension_registry() instead of the registry used by HostRuntimeServices::new in factory.rs. Even if third-party activation is a follow-up, this keeps hook activation as a sidecar reconstruction rather than a natural extension of the runtime's canonical extension registry, and the surrounding comment now contradicts itself by calling it this tenant's registry and then builtin-only. The cleaner move is to make the extension registry a shared composition artifact: build the hook factory at the same boundary that builds HostRuntimeServices, or carry the Arc<ExtensionRegistry> through RebornServices so capability dispatch and hook activation cannot drift.
There was a problem hiding this comment.
Fixed in ce0c127. The extension registry is now a shared composition artifact rather than a rebuilt sidecar:
build_local_devbuilds oneArc<ExtensionRegistry>and hands it to BOTHHostRuntimeServices::new(capability dispatch) andRebornLocalRuntimeServices.extension_registry.build_reborn_runtimenow doesArc::clone(&local_runtime.extension_registry)and feeds that tobuild_hook_dispatcher_builder_factory— the exactArccapability dispatch resolves through. No morebuiltin_extension_registry()rebuild at the call site (the import is gone).
So hook activation and capability dispatch can no longer drift — there is one registry instance per build. The self-contradicting "this tenant's registry / builtin-only" comment is replaced with wording that says it consumes the canonical registry, which today carries only the builtin/host-bundled package; the moment installed third-party packages are inserted into that same registry upstream, the loader picks them up here with no call-site change. Live third-party activation remains a deliberate follow-up, but it now follows the canonical registry, not a separate wiring path.
… 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>
|
Pushed ce0c127 addressing the thermo-nuclear re-review. Summary of all six items: #1 — canonical registry (was: sidecar reconstruction, runtime.rs:839). The extension registry is now a shared composition artifact. #2 — env boundary (BLOCKER). Already fixed in 1e618d0 ( #3 — factory machinery / the #4 — predicate counter scoping (BLOCKER). Applied the decision: counters are intentionally TENANT-scoped (keyed #5 — empty production catalog. Already fixed in 1e618d0 (no shipped no-op; test-only hooks exercise the machinery). Confirmed. #6 — tests reimplement the loader. Added Checks: Flagged follow-up (NOT one of the six items, not introduced here): the new runtime test surfaced that with the flag ON the standalone local-dev runtime does not reach |
|
@serrrfirat re-review addressed in ce0c127 (inline replies posted). The structural items: (1) hook activation now consumes the SAME |
|
Re-verification pass on `ce0c12749` — every finding from both CHANGES_REQUESTED reviews is addressed at this SHA. No further code changes were needed; this pass confirms the fixes are present, correct, and green. Headline blocker: predicate-state boundary (made explicit)Chosen scope: predicate counter state is TENANT-scoped and deliberately shared across runs — NOT run-scoped. Rationale: rate-limit / value-cap counters are keyed `(hook, tenant, capability)` with no `run_id`, so a run-scoped counter would reset every run and could never enforce a cross-run budget. The durable predicate backends (#3933/#3936) are likewise keyed by tenant/identity, so tenant-scoping is the only coherent boundary. How it's now unambiguous:
Per-finding disposition (all at ce0c127)
Production activation stays behind `HooksActivationConfig` (default OFF), fail-closed on malformed manifests, routed through the established hook dispatch pipeline. Verification (IRONCLAW_DISABLE_OS_KEYCHAIN=1)
Tip: `ce0c12749` (unchanged — fixes already landed in the prior activation commits). |
…arantine, 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>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Activate hook framework in production behind HOOKS_ENABLED flag (default OFF)
Stats: 16 findings (from 16 raw, 16 after dedup) across 8 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability. Reviewers failed: none. Body-only: 2
Bugs
- Medium SurfaceBackedProviderResolver silently returns None before surface init (
loop_driver_host.rs:246-269, confidence 75) - Medium Event subscription silently dropped when dispatcher factory produces None (
loop_driver_host.rs:1242-1263, confidence 60) - Medium ReadScope validation may silently fail due to cross-type comparison (
loop_driver_host.rs:563-592, confidence 55) - Low adaptive_poll_interval truncates u128 nanos to u64 (
loop_driver_host.rs:730-737, confidence 60) - Low max_messages clamped to at least 1 may override caller intent (
loop_driver_host.rs:1195, confidence 55) - Low InvocationCount uses strict greater-than semantics after recording (
evaluator.rs:209-213, confidence 60) - Low TOML re-serialization may cause false-positive size limit rejections (
v2.rs:854-858, confidence 60) - Low NumericSum may not handle integer vs string JSON representations consistently (
evaluator.rs:215-278, confidence 55) - Low HookInstallPlan::rebuild uses unreachable!() that aborts process on impurity (
hooks.rs:438-448, confidence 55)
Performance
- Medium InMemoryPredicateStateBackend uses per-map Mutex causing contention (body-only — no diff position) — all concurrent record_invocation calls serialize through the same mutex.
- Low HookInstallPlan::rebuild deep-clones all HookManifestEntry values per run (
hooks.rs:273-295, confidence 85)
Tests
- Medium HookEntryTooLarge error path has no test coverage (
v2.rs:313-319, confidence 100) - Medium Non-table hook entry rejection not tested (
v2.rs:286-290, confidence 75) - Low Empty or whitespace-only hook id rejection untested (
v2.rs:299-304, confidence 75)
Conventions
- Medium LocalDevYolo profile variant removed from public enum without rationale (body-only — no diff position)
- Medium Turn event projection wiring removed from build_reborn_runtime without rationale (
runtime.rs:791-794, confidence 55)
Security, Conventions, Local Patterns, Maintainability
No findings.
| /// predicate counter, and any registry mutations done while a run is | ||
| /// active do not leak into the next run. Default behavior (no factory) is | ||
| /// unchanged from the pre-hooks shape. | ||
| /// dispatcher per host build means **dispatcher-local** state — |
There was a problem hiding this comment.
Medium — SurfaceBackedProviderResolver silently returns None before surface init.
provider_for silently returns None when the capability surface has not been populated yet. OwnCapabilities-scoped hooks evaluating before surface init will never fire because provider stays None.
Fix: Add a warn! log when provider_for returns None before surface is initialized, or initialize the surface eagerly.
Also flagged by: bugs/Medium (Event subscription silently dropped), bugs/Medium (ReadScope validation may silently fail), bugs/Low (adaptive_poll_interval truncation), bugs/Low (max_messages clamping)
There was a problem hiding this comment.
Out of scope for this PR. The SurfaceBackedProviderResolver/provider_for, event-subscription, ReadScope, adaptive_poll_interval, and max_messages lines in loop_driver_host.rs are not changed by this branch — they appear in the inflated review diff because the bot reviewed against a stale reborn-integration base. The only real change to crates/ironclaw_reborn/src/loop_driver_host.rs in this PR is doc-comment wording pinning the tenant-scoped predicate-counter boundary (no logic). Confirmed via the merge-base diff.
| /// used by tests that need to pre-seed backend state (e.g. saturate a | ||
| /// per-key window to drive the fail-closed overflow path) and by | ||
| /// future host wiring that swaps in a durable backend. | ||
| /// per-key window to drive the fail-closed overflow path). |
There was a problem hiding this comment.
Low — InvocationCount uses strict greater-than semantics after recording.
With max=2 this allows exactly 2 invocations and denies the 3rd. Some callers may expect inclusive <= semantics.
Fix: Document the strict greater-than semantics, or switch to >= if inclusive is intended.
Also flagged by: bugs/Low (NumericSum representation)
There was a problem hiding this comment.
Intentional — documented now. record_invocation returns the count including the current invocation, so count > max is an inclusive ceiling: with max=2 the 1st and 2nd are allowed and the 3rd denied. This is the deliberate tenant-scoped cap semantics pinned by the InvocationCount cap test. Added an inline comment at the comparison site (e694740) making the inclusive-allow / deny-on-overflow contract explicit.
| /// Maximum serialized size, in bytes, of a single `[[hooks]]` entry's body. | ||
| /// | ||
| /// The hook-declaration section is carried as an opaque, structurally-typed | ||
| /// TOML payload (see [`HookSectionEntryV2`]) so `ironclaw_extensions` never |
There was a problem hiding this comment.
Low — TOML re-serialization may cause false-positive size limit rejections.
toml::to_string(&raw) re-serializes hook entry back to TOML for size measurement. The canonical form may be larger than the original input.
Fix: Measure the original TOML bytes instead of re-serialized form.
There was a problem hiding this comment.
Intentional. The re-serialized canonical TOML is exactly the raw_toml payload the composition loader re-parses (stored on HookSectionEntryV2), so measuring its length measures the real retained byte cost, not a proxy. Measuring the original slice would size-check bytes we never keep. The 8 KiB bound has ample headroom over any legitimate hook body, so canonical-form expansion is not a false-positive risk in practice. Left as-is.
| //! compiled once (parse + validate the full install set, fail-closed), | ||
| //! returned to the runtime to pass to | ||
| //! `RebornLoopDriverHostFactory::with_hook_dispatcher_builder_factory`. The | ||
| //! factory closure calls [`HookInstallPlan::rebuild`] to mint a *fresh* |
There was a problem hiding this comment.
Low — HookInstallPlan::rebuild uses unreachable!() that aborts process on impurity.
rebuild uses unreachable!() assuming replay is infallible. An impure closure would abort instead of giving a graceful error.
Fix: Replace unreachable!() with expect() or a Result that surfaces the failure gracefully.
Also flagged by: performance/Low (HookInstallPlan deep-clones all entries per run)
There was a problem hiding this comment.
Declined. unreachable!() is acceptable here and is our convention for type-carried invariants. HookInstallPlan can only exist if compile already proved the identical install sequence composes against an identical fresh-empty builder; rebuild replays that exact deterministic sequence, so the error arm is genuinely unreachable rather than a swallowed runtime failure (the rebuild doc comment spells this out). The per-run HookManifestEntry clone is the deliberate cost of per-run dispatcher freshness (the preserved isolation boundary) and is cheap relative to dispatch; entries are parsed/validated once at compile time, never per run.
| #[error( | ||
| "hook entry '{id}' body is {bytes} bytes, exceeding the per-entry maximum of {max} bytes" | ||
| )] | ||
| HookEntryTooLarge { |
There was a problem hiding this comment.
Medium — HookEntryTooLarge error path has no test coverage.
The raw_toml.len() > MAX_HOOK_ENTRY_BYTES branch is never exercised.
Fix: Add test covering HookEntryTooLarge.
There was a problem hiding this comment.
Fixed. Added manifest_rejects_oversized_hook_entry exercising the HookEntryTooLarge branch (pads a hook body past MAX_HOOK_ENTRY_BYTES). Commit e694740.
| /// Tune cautiously — raising this also raises peak loader memory. | ||
| pub const MAX_MANIFEST_BYTES: usize = 256 * 1024; | ||
|
|
||
| /// Maximum number of `[[hooks]]` entries a single manifest may declare. |
There was a problem hiding this comment.
Medium — Non-table hook entry rejection not tested.
The non-table hook entry rejection branch is never exercised.
Fix: Add test covering non-table hook entry rejection.
There was a problem hiding this comment.
Fixed. Added manifest_rejects_non_table_hook_entry (a hooks = ["not-a-table"] element) asserting the "must be a TOML table" rejection. Commit e694740.
| /// `HookManifestEntry`, validates it, and installs it through | ||
| /// `HookRegistrar::install` at the `Installed` trust tier. | ||
| /// | ||
| /// What this crate validates: the entry is a table, carries a non-empty |
There was a problem hiding this comment.
Low — Empty or whitespace-only hook id rejection untested.
The trim().is_empty() check is never exercised.
Fix: Add test covering whitespace-only hook id rejection.
There was a problem hiding this comment.
Fixed. Added manifest_rejects_whitespace_only_hook_id asserting the "id must not be empty" rejection. Commit e694740.
| /// `ironclaw_reborn` host-factory tests (which install hand-built | ||
| /// dispatcher factories), this proves the real composition root wires the | ||
| /// right registry + config: with the flag ON the runtime must still build | ||
| /// and complete a turn (the empty first-party catalog + builtin-only |
There was a problem hiding this comment.
Medium — Turn event projection wiring removed from build_reborn_runtime without rationale.
The .with_turn_events() wiring was removed. No rationale provided.
Fix: Document whether turn event projection is intentionally deferred or restore the wiring.
There was a problem hiding this comment.
Out of scope / stale. with_turn_events / turn-event projection wiring is not touched by this branch — the removal shows up only in the inflated review diff against a stale reborn-integration base (the real change lives in unrelated upstream commits, e.g. the turn_event_publisher module removal). The composition runtime.rs change in this PR adds the hook-dispatcher-factory wiring; grep for turn_event/with_turn_events in this PR diff returns nothing. No action.
…Count 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>
|
Addressed @henrypark133's latest review (4367870023). New tip: Triage summary — the review diff was inflated by a stale Fixed (genuine test gaps):
Documented (intentional, behavior unchanged):
Declined with rationale:
Out of scope / stale (not in this branch's diff — base divergence):
Verified (IRONCLAW_DISABLE_OS_KEYCHAIN=1): |
|
Review of #3938: activate hook framework in production Overall: solid activation design with correct rollout safety. No critical bugs found. Low —
fn rebuild(&self) -> HookDispatcherBuilder {
match self.try_build_once() {
Ok(builder) => builder,
Err(error) => unreachable!(...),
}
}The infallibility claim relies on Fix: Add a compile-time doc-comment constraint on Nit —
The constant intentionally copies rather than re-exports (clean-boundary contract documented). Risk: if Fix: Add a test Everything else — flag-off default by |
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review — feat(hooks): activate hook framework in production behind HOOKS_ENABLED flag
Reviewers: security · bugs · performance · tests · conventions
Findings: 1 Medium · 1 Low · 1 Nit
Event: COMMENT
Summary
Strong implementation. The core architecture holds up well:
- Feature-flag default-OFF contract is tight and the test suite pins it (
config_defaults_to_disabled) - Trust attenuation is enforced by construction (Installed tier, registrar never mints Allow/Gate/Mutator)
- Per-tenant isolation is correct —
build_hook_dispatcher_builder_factory_withcreates a freshInMemoryPredicateStateBackendper call, so no cross-tenant backend sharing - Fail-closed everywhere: malformed manifest →
RebornBuildError::InvalidConfig, not a panic HookInstallPlan::rebuildunreachable!is sound — replay of a deterministic, pre-validated install sequence from a fresh-empty builder- The per-tenant-scoped / per-run-fresh dispatcher split is well documented and the tests pin both halves
Findings
🟡 Medium — Missing hooks_config assertion in build_runtime_input caller tests
File: crates/ironclaw_reborn_cli/src/runtime.rs line 254
Anchor: .claude/rules/testing.md — "Test Through the Caller, Not Just the Helper"
HooksActivationConfig::from_env() is a transform whose return value gates dispatcher composition. build_runtime_input is the wrapper between the env-read and the side effect. The testing rule requires a caller-level test when: (1) a helper gates a side effect, (2) there is a wrapper between them, and (3) the caller computes inputs from context (here: the process environment).
The existing tests for build_runtime_input (build_runtime_input_maps_configured_cli_identity, build_runtime_input_for_run_rejects_default_project, build_runtime_input_for_serve_accepts_default_project) verify identity and project-rejection but none assert the hooks field. A regression that silently dropped with_hooks_config or changed its default would not be caught.
Fix: Add to build_runtime_input_maps_configured_cli_identity:
assert!(!runtime_input.hooks.is_enabled(), "hooks must be disabled by default when HOOKS_ENABLED is unset");🔵 Low — Warning text "LRU cap is shared across tenants" is inaccurate for the v1 composition
File: crates/ironclaw_reborn_composition/src/hooks.rs line 482
Anchor: CLAUDE.md "Comments for non-obvious logic only" + in-code docs accuracy
warn_in_memory_backend_active_in_production() emits:
"predicate evaluator is using the in-memory backend: replay dedup is process-local and the LRU cap is shared across tenants."
But in this PR's composition, build_hook_dispatcher_builder_factory_with creates a fresh InMemoryPredicateStateBackend::new() per call — one per build_reborn_runtime per tenant. The backend is NOT shared across tenants. The only real limitation today is process-local replay dedup (no multi-host).
Misleading operators into treating cross-tenant LRU pressure as the urgent concern could cause misplaced operational urgency. The text was written for a hypothetical global-backend scenario and predates this PR's per-tenant construction; calling it from production code surfaces the inaccuracy.
Fix: Update warn_in_memory_backend_active_in_production in evaluator.rs:
tracing::warn!(
"predicate evaluator is using the in-memory backend: replay dedup is \
process-local (event_id replay not multi-host safe). In this composition \
the backend is per-tenant so there is no cross-tenant LRU sharing. \
Multi-host deployments MUST swap to the durable backend \
(see crates/ironclaw_hooks/docs/successors/03-persistent-counter.md)."
);⚪ Nit — with_backend and with_state_backend have identical bodies
File: crates/ironclaw_hooks/src/evaluator.rs lines 85–97
Anchor: CLAUDE.md "Keep functions focused, extract helpers when logic is reused"
Both constructors are Self { backend }:
#[cfg(test)]
pub(crate) fn with_backend(backend: Arc<dyn PredicateStateBackend>) -> Self { Self { backend } }
pub fn with_state_backend(backend: Arc<dyn PredicateStateBackend>) -> Self { Self { backend } }The separation of test API from production API is intentional and reasonable. Since the body is a one-liner, divergence risk is minimal. A comment or delegation would make the intent explicit.
Fix (optional): Add a // Intentionally identical to with_backend (test-only); both wrap the same field. comment, or have with_backend delegate to with_state_backend.
| // Resolve the hook-activation flag from the environment ONCE here at the | ||
| // edge (default OFF), then thread the typed config down. The composition | ||
| // root no longer reads `HOOKS_ENABLED` itself. | ||
| .with_hooks_config(HooksActivationConfig::from_env()); |
There was a problem hiding this comment.
🟡 Medium — Missing hooks_config assertion in caller test (.claude/rules/testing.md)
The existing build_runtime_input_maps_configured_cli_identity test exercises build_runtime_input but doesn't assert the hooks field. Per the "Test Through the Caller" rule: from_env() is a transform gating dispatcher composition, build_runtime_input is the wrapper. A regression that silently drops with_hooks_config or changes its default wouldn't be caught.
// Add to build_runtime_input_maps_configured_cli_identity:
assert!(
!runtime_input.hooks.is_enabled(),
"hooks must be disabled by default when HOOKS_ENABLED is unset"
);There was a problem hiding this comment.
Confirmed: build_runtime_input_maps_configured_cli_identity (runtime.rs:418) drives the caller and asserts identity fields but never touches hooks. The default-OFF flag is the rollout-safety contract, and with_hooks_config(HooksActivationConfig::from_env()) (runtime.rs:254) is exactly the wrapper-threads-a-flag shape this rule guards. Will fix: add assert!(!runtime_input.hooks.is_enabled(), "hooks must be disabled by default when HOOKS_ENABLED is unset") to that existing test so a dropped with_hooks_config or flipped default fails here.
There was a problem hiding this comment.
Fixed in 218a1de: extended build_runtime_input_maps_configured_cli_identity with two assertions on the hooks field. Threading: runtime_input.hooks == HooksActivationConfig::from_env() (proves with_hooks_config is not dropped; verified via TDD that removing the wiring fails this under HOOKS_ENABLED=1). Default-OFF: !runtime_input.hooks.is_enabled() when HOOKS_ENABLED is unset, guarded to skip if the env exports the flag.
| // (see the module-level "Predicate counter scoping" note). | ||
| let backend: Arc<dyn PredicateStateBackend> = Arc::new(InMemoryPredicateStateBackend::new()); | ||
| let evaluator = Arc::new(PredicateEvaluator::with_state_backend(Arc::clone(&backend))); | ||
| evaluator.warn_in_memory_backend_active_in_production(); |
There was a problem hiding this comment.
🔵 Low — Warning text "LRU cap is shared across tenants" is inaccurate for v1 composition (CLAUDE.md: comments accuracy)
warn_in_memory_backend_active_in_production() says "the LRU cap is shared across tenants" but this module creates InMemoryPredicateStateBackend::new() fresh per call — one per build_reborn_runtime per tenant. No cross-tenant backend sharing exists here. The only real limitation is process-local replay dedup.
The warning was written for a hypothetical global-backend scenario; calling it from per-tenant production code surfaces the inaccuracy and could mislead operators.
Fix: Update the message in evaluator.rs to say replay dedup is process-local (no multi-host safety), and clarify the backend is per-tenant in this composition.
There was a problem hiding this comment.
Correct that this call site is per-tenant: build_hook_dispatcher_builder_factory_with mints a fresh InMemoryPredicateStateBackend::new() (hooks.rs:480) and build_reborn_runtime runs once per identity, so the LRU is not shared across tenants on this path — the live limitation here is process-local replay dedup. The wording is accurate only as a statement about a hypothetical global/shared backend (the docstring references the #3635 multi-host scenario). The fix lives in ironclaw_hooks::evaluator::warn_in_memory_backend_active_in_production, a generic helper shared with other (non-per-tenant) callers, so I'll reword it in the durable-backend follow-up (#3933) rather than this activation PR — the inaccuracy is operator-log cosmetics, not a behavior bug. Defer.
There was a problem hiding this comment.
Fixed in 65fd158: reworded warn_in_memory_backend_active_in_production (text and rustdoc) in this PR rather than deferring. The warn! now states the real limitation — process-local replay dedup for multi-host deployments — and explicitly notes the backend is constructed per-tenant in this composition, so the LRU cap is not shared across tenants. Pointer to crates/ironclaw_hooks/docs/successors/03-persistent-counter.md retained.
| /// for a durable one (Postgres / libSQL, #3933 + follow-ups) without | ||
| /// changing the evaluator's predicate semantics. The composition layer | ||
| /// passes the per-tenant backend here when activating the hook framework. | ||
| pub fn with_state_backend(backend: Arc<dyn PredicateStateBackend>) -> Self { |
There was a problem hiding this comment.
⚪ Nit — with_backend and with_state_backend have identical bodies (CLAUDE.md: keep functions focused)
Both constructors are Self { backend }. The separation of test API (#[cfg(test)] pub(crate)) from production API (pub) is intentional. Since the body is a trivial one-liner, divergence risk is minimal.
// Optional: have with_backend delegate, or add a comment:
// Intentionally matches `with_backend` (test-only); both wrap the same field.
pub fn with_state_backend(backend: Arc<dyn PredicateStateBackend>) -> Self {
Self { backend }
}There was a problem hiding this comment.
The duplication is intentional and load-bearing. with_backend is #[cfg(test)] pub(crate) (evaluator.rs:84) for pre-seeding backend state in fail-closed overflow tests; with_state_backend is pub (evaluator.rs:96) as the production durable-backend swap seam (#3933). Collapsing them either exposes the test-only constructor to downstream crates or compiles the production seam out under #[cfg(test)]. Different visibility, different audience, different rustdoc — the identical one-line body is incidental, not a focus violation. Leaving as-is.
There was a problem hiding this comment.
Addressed in 65fd158: kept the two constructors separate (the test-only #[cfg(test)] pub(crate) visibility vs the pub production seam is intentional, as noted) but made with_backend delegate to Self::with_state_backend(backend) with an intent comment, so the construction stays in lockstep if it ever grows beyond Self { backend }. No visibility change.
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>
#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>
|
@serrrfirat All June 3 findings are addressed as of |
|
@henrypark133 This one is ready for your re-review as well — all findings from the June 3 review pass are addressed (see the summary comment above for commits), and your review request is still pending on it. |
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent, re-review)
Intent: Activate hooks framework in production behind HOOKS_ENABLED flag (#3934).
Stats: 6 new findings (2 Medium, 4 Low) after deduping against 45 prior comments. 8 reviewers run.
Previously raised — author has sound reply, no re-block
runtime.rsHOOKS_ENABLED read at build_reborn_runtime — fixed in1e618d076via typedHooksActivationConfigfield onRebornRuntimeInput.hooks.rs:481in-memory backend rate-limit bypass / multi-host — author reworded warn in65fd15841; resolved ince0c12749(predicate counters intentionally tenant-scoped).hooks.rsinstall-plan duplicate iterate-and-parse machinery — replaced with typedHookInstallPlanince0c12749.hooks.rs:33unreachable!()inrebuild— author declined: type-carried invariant,compileproves replay is infallible.extensions/v2.rs:583HookEntryTooLargetest gap — addedmanifest_rejects_oversized_hook_entryine69474012.runtime.rs:254hooks_config assertion in caller test — added in218a1de59.hooks.rs:176first-partyNoOpObserverHookdesign (chatgpt-codex bot) — removed from production catalog in1e618d076.
Tests
- Medium
MAX_MANIFEST_HOOKSexact-boundary (32 accepted) not tested (extensions/tests/extension_contract.rs:1281, conf 75) — only the over-limit case is exercised; an off-by-one regression to>=would not be caught. See inline. - Low
HooksActivationConfig::from_env()with truthyHOOKS_ENABLEDnot directly tested (hooks.rs:140, conf 60) —is_truthy()is covered but thestd::env::varwrapper around it is not. See inline.
Performance
- Medium
HookInstallPlanreplay clones all extension entries on every rebuild (hooks.rs:418, conf 72) —set.entries.clone()per per-run host build. See inline. - Low Hook entry TOML round-trips through serialize-then-re-parse from
v2.rsvalidation to composition load (extensions/src/v2.rs:841, conf 65). See inline.
Conventions
- Low
HOOKS_ENABLEDenv var missing from.env.example(conf 85). Body-only — no diff context for.env.example. Add it alongsideSANDBOX_ENABLEDandENGINE_V2.
Local patterns
- Low
E2E_NOOP_OBSERVER_PATHdoc-comment claims canonical alignment with composition module's first-party observer, butNoOpObserverHookwas removed from production in1e618d076(loop_driver_host.rs:2206, conf 75). See inline.
| #[test] | ||
| fn manifest_rejects_too_many_hooks() { | ||
| let mut hooks = String::new(); | ||
| for i in 0..(MAX_MANIFEST_HOOKS + 1) { |
There was a problem hiding this comment.
Medium — manifest_rejects_too_many_hooks exercises MAX_MANIFEST_HOOKS + 1 (33 entries). The > comparison in validate_hook_entries means exactly 32 entries must be accepted — an off-by-one switch to >= would reject 32 and this test would not catch it.
Fix: Add manifest_with_exactly_max_hooks_is_accepted constructing 32 valid [[hooks]] entries and asserting the manifest parses without error.
New finding — not raised in prior reviews.
| //! construction: the registrar only ever calls `install_installed_*`, so an | ||
| //! extension hook can never mint `Allow` / `Gate` / `Mutator` without an | ||
| //! explicit per-extension grant. | ||
| //! 4. **The per-run dispatcher builder factory** — a [`HookInstallPlan`] |
There was a problem hiding this comment.
Medium — install_extension_sets calls set.entries.clone() to pass owned Vec<HookManifestEntry> into registrar.install on every rebuild. With third-party installed extensions, each capability invocation that spawns a new host (which calls rebuild) allocates O(total_hook_entries) clones. The registrar .install signature takes entries by value, forcing the clone.
Fix: Accept &[HookManifestEntry] in the registrar install path so HookInstallPlan can lend the already-validated entries rather than cloning them per rebuild.
New finding — not raised in prior reviews.
| /// Tune cautiously — raising this also raises peak loader memory. | ||
| pub const MAX_MANIFEST_BYTES: usize = 256 * 1024; | ||
|
|
||
| /// Maximum number of `[[hooks]]` entries a single manifest may declare. |
There was a problem hiding this comment.
Low — validate_hook_entries serializes each raw toml::Value to a canonical TOML string solely for size measurement, then stores that string. Composition's project_extension_install_sets then calls toml::from_str on each stored string. The flow is parse → serialize → store → deserialize — two encode/decode round-trips per entry when only one is structurally needed.
Fix: Store a pre-validated size (bytes: usize) alongside the toml::Value, or pass the serialized string through to composition so the second decode is the only one. Cold-path cost, but easy to remove.
New finding — not raised in prior reviews.
| //! deliberately **EMPTY**: no real first-party builtin hook has been | ||
| //! productized, so we ship none. A production type + install path + behavior | ||
| //! for a hook that does nothing is scaffolding, not a deliverable. The | ||
| //! activation machinery is exercised end-to-end with test-only hooks (see |
There was a problem hiding this comment.
Low — truthy_tokens_enable_only_canonical_values covers the is_truthy() helper directly, but from_env() wraps it with std::env::var. No test sets HOOKS_ENABLED=true and asserts from_env().is_enabled() == true, so a bug that drops the env-var read (e.g. always returning Err) would not be caught.
Fix: Add from_env_returns_enabled_when_hooks_enabled_set_to_true setting the env var (with cleanup) and asserting from_env().is_enabled().
New finding — not raised in prior reviews.
|
|
||
| /// Canonical path matching the composition module's first-party no-op observer | ||
| /// so the e2e exercises the same builtin identity. | ||
| const E2E_NOOP_OBSERVER_PATH: &str = "ironclaw_reborn_composition::hooks::NoOpObserverHook"; |
There was a problem hiding this comment.
Low — The doc-comment on E2E_NOOP_OBSERVER_PATH claims it matches the composition module first-party no-op observer canonical path. In 1e618d076, NoOpObserverHook was removed from the production first-party catalog (install_first_party_hooks is now a no-op / empty). The constant now refers to a type that no longer exists at that path; the only NoOpObserverHook left is the composition test-only one at ...::hooks::tests::NoOpObserverHook.
Fix: Update the comment to clarify this is a test-local host-plumbing double, not aligned with any production type. Cross-reference the existing note at line 2304 about host-plumbing doubles.
New finding — not raised in prior reviews.
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
# Conflicts: # crates/ironclaw_product_workflow/tests/inbound_turn_contract.rs # crates/ironclaw_product_workflow/tests/support/planned_agent_loop.rs # crates/ironclaw_reborn/src/runtime.rs # crates/ironclaw_reborn/tests/loop_driver_host.rs # crates/ironclaw_reborn_composition/tests/product_live_adapters.rs # tests/support/reborn/harness.rs
…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>
…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>
…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>
3 participants
Activates the (previously dormant) hook framework in the live Reborn capability-invocation path, gated behind a new
HOOKS_ENABLEDflag that is default OFF. Resolves #3934.Ships dark. The flag defaults OFF, so this PR changes zero production behavior until the flag is deliberately flipped. With the flag OFF,
build_default_planned_runtimecomposes noHookDispatcherand the runtime behaves exactly as it did before hooks existed.Per-piece summary
Extension hook-declaration surface (
ironclaw_extensions) — adds a[[hooks]]section to the production v2 manifest (ExtensionManifestV2+ projectedExtensionManifest). Each entry is carried as a structurally-typedHookSectionEntryV2DTO (alocal_id+ the entry body re-serialized to canonical TOML). Clean boundary:ironclaw_extensions(substrate) does not depend onironclaw_hooks; it never imports the hook predicate vocabulary. Parse-time structural bounds:MAX_MANIFEST_HOOKS(32),MAX_HOOK_ENTRY_BYTES(8 KiB), non-empty uniqueid.#[serde(default)]keeps every existing manifest valid.Manifest → registry loader (
ironclaw_reborn_composition::hooks) — the single composition-layer seam that projects eachHookSectionEntryV2into a typedironclaw_hooks::HookManifestEntry(toml::from_str) and installs it throughHookRegistrar::install. This is where the hook vocabulary lives. Trust attenuation is enforced by construction: the registrar only ever callsinstall_installed_*, so extension-declared hooks are pinned to the Installed tier and cannot mintAllow/Gate/Mutatorwithout an explicit per-extension grant. Fail-closed: a malformed manifest hook fails the build, never silently drops.First-party builtin hooks — a single illustrative no-op observer (
NoOpObserverHook), installed regardless of extensions at theBuiltintier. It ships dark (observers can't affect outcomes; this one records nothing). The production first-party catalog is TBD by design — no real first-party deny/policy hook has been productized, and this PR deliberately does not invent one. The deliverable is the activation machinery, not a hook catalog.HookRegistry construction, per-tenant — built inside
build_reborn_runtime, which runs once per identity/owner (onetenant_idper call). The registry,PredicateEvaluator, and per-run dispatcher closure are all tenant-local — no global registry, so one tenant's hooks/counters never apply to another (the [codex] Add Reborn multi-tenant isolation contract tests #3890 isolation contract).HookDispatcher composition — a per-tenant
PredicateEvaluatorover the in-memory predicate-state backend, swappable via the new publicPredicateEvaluator::with_state_backendso the durable Postgres/libSQL backend (feat(hooks): PostgresPredicateStateBackend (durable backend PR 2/4, replaces #3932) #3933 + follow-ups) drops in without touching this wiring. The full install set is validated once at composition time (fail-closed). Milestone sink is attached per-run by the host factory.Runtime wiring —
DefaultPlannedRuntimePartsgains an optionalhook_dispatcher_builder_factory;build_default_planned_runtimecalls.with_hook_dispatcher_builder_factory(...)when present. A per-run builder factory (not a captured dispatcher) is used so each host build mints a fresh dispatcher (no cross-run poison/counter leak) and telemetry attribution is per-run — the feat(reborn): add ironclaw_hooks framework foundation (#3524) #3573 capture-and-stick lesson.Feature flag — default OFF —
HooksActivationConfig, resolved fromHOOKS_ENABLED(only1/true/yes/onenable; unset/anything else = OFF). OFF ⇒ factory isNone⇒ no dispatcher composed ⇒ zero behavior change. Fail-closed: with the flag ON, a registry/backend construction failure fails the build loudly (empty/first-party-only is the one legitimate non-error empty state).End-to-end tests through the production composition (
build_default_planned_runtime, not the test-only factory) — flag OFF (capability unaffected, inner port reached); flag ON first-party-only (outcome unchanged); flag ON extension-declared deny hook (denied through the composed runtime, installed at the Installed tier, inner port never reached); per-tenant isolation (tenant A's deny doesn't fire for tenant B's separate runtime).Rollout-safety story
Default OFF is the hard contract: this is a deliberate, feature-flagged activation (default off → canary → on), not a hard cutover. With the flag off there is no dispatcher in the hot path and no measurable change. Flipping
HOOKS_ENABLED=truecomposes the dispatcher per-run and lets hooks gate real capability invocations.In-memory backend for now (durable follow-up)
The predicate evaluator uses the in-memory state backend in v1. It is process-local (replay dedup) and its LRU is shared across tenants — fine for single-host/canary, not multi-host. The backend is swappable (
PredicateEvaluator::with_state_backend); the durable Postgres/libSQL backend (#3933 + parity suite) drops in here. A startupwarn!is emitted when the in-memory backend is active.Deferred follow-ups (noted, not blocking)
SecurityAuditSink/with_hook_security_audit_sinkare not yet onreborn-integration. The deny-path-audit e2e assertion is intentionally deferred and lands with the audit-sink wiring once feat: wire SecurityAuditSink into obligation handler + hook deny paths #3922 merges. No hard dependency on feat: wire SecurityAuditSink into obligation handler + hook deny paths #3922 is introduced.HookGateRouterexists in the composition path yet; the fail-closed default (PauseApproval surfaces as Denied) applies. Wiringwith_hook_gate_ref_factory_builderto a real router is a follow-up.Verification
cargo fmt --all --check— cleancargo clippy --workspace --tests --all-features— zero warningscargo testgreen forironclaw_hooks(258),ironclaw_extensions(35, +7 new hook-section tests),ironclaw_reborn(loop_driver_host93 incl. 4 new e2e,hooks_integration45),ironclaw_reborn_composition(lib 33, +5 new activation tests),ironclaw_product_workflowcargo build -p ironclaw_reborn_composition --features postgresand--features libsql— both compile (dual-backend rule; the swappable-backend seam touches persistence-adjacent code)cargo build --workspace+cargo test --workspace --no-run— clean🤖 Generated with Claude Code