hooks: address henrypark approval-with-followup items from #3911/#3912/#3913 - #3921
Merged
Merged
Conversation
Address henrypark133 approval-with-followup items from PR #3912: - L1: remove dead empty-id guard in HookManifestEntry::validate. HookLocalId::new now rejects empty strings at construction, so manifest deserialization fails before validate() is ever called. - L2: add AsRef<str>, From<Self> for String, and into_inner() to ExtensionId and HookLocalId per the canonical newtype template in .claude/rules/types.md. into_string() is retained as a thin alias for compatibility; new code should prefer into_inner(). - L3: document why the builtin_id_distinct_from_extension_id test fixture substitutes "path.module" for the original "path::module" (the new grammar rejects colons in HookLocalId). No behavioral change.
…after allowed entries henrypark133 M1 on PR #3911: in `HookedLoopCapabilityPort::invoke_capability_batch`, when Phase 1 produced a mix of Pending (hook-allowed) and Resolved (hook-suspension) slots with the suspension appearing AFTER an allowed entry and `stop_on_first_suspension = true`, the merge loop initialized `stopped_on_suspension` from `stopped_in_preflight` and broke after the very first iteration. Result: trailing Resolved suspension slots never fired their `AfterCapability` observer and never surfaced in the merged `outcomes` vec, violating the per-entry observer contract from PR #3573 (serrrfirat P2 #3). Fix: continue iterating the merge loop so every slot fires its observer and every Resolved outcome is pushed. Only Pending slots are dropped after a stop (their inner work was already short-circuited in Phase 1 or by an early inner-port stop), tracked via `pending_after_stop`. Re-pop guard on `inner_outcomes.pop()` keeps the previous "inner stopped early on its own suspension" semantics: pending slots without an inner outcome are dropped, but the loop continues so any trailing Resolved observers still fire. New regression test `batch_invocation_fires_observer_for_hook_suspended_entry_after_allowed_entry_with_stop_on_first_suspension` pins the behavior: `[alpha=hook-allowed, beta=hook-suspension]` with `stop_on_first_suspension = true` produces a 2-entry `outcomes` vec (Completed alpha + ApprovalRequired beta), fires the observer twice, and only sends alpha to the inner port. Verified TDD-style: the test fails on the pre-fix code with `outcomes.len() = 1`.
henrypark133 L1 on PR #3913: `resolve_arguments` measured the post-resolver JSON payload by calling `serde_json::to_vec(&value)` and discarding the `Vec<u8>` once its length was checked. The `SanitizedArguments::from_json` constructor on the happy path sanitizes the in-memory `serde_json::Value` directly without re-serializing, so the materialized buffer was pure overhead. Switch the size measurement to `serialized_len`, a counting `io::Write` adapter that streams `serde_json::to_writer` into a u64 counter — saves one Vec<u8> allocation and the matching drop per resolved invocation. Behavior is identical: the same JSON encoding rules drive both writers, the cap check still fires when the encoded length exceeds `MAX_PREDICATE_INPUT_BYTES`, and serialization errors still fail closed. No new tests required; existing `dispatch_fails_closed_when_input_exceeds_max_bytes` and the ordering / lazy-probe tests exercise this path.
henrypark133 L2 on PR #3913: add `before_capability_needs_input_returns_true_when_any_active_binding_needs_input`. Installs two BeforeCapability bindings on the same scope (Global) — one `needs_input() = false`, one `needs_input() = true` — and asserts both ends of the short-circuit: 1. `HookDispatcher::before_capability_needs_input(None)` returns true. 2. Driving `HookedLoopCapabilityPort::invoke_capability` with an instrumented `ProbingResolver` confirms the resolver IS consulted exactly once — i.e. the short-circuit fires through the call site, not just the helper. This pins the "any input-needing binding wins" contract end-to-end so a future change to the dispatcher's probe (or to the middleware's lazy-probe gate) can't silently regress to short-circuiting on the first binding only. Also tightens the merge-loop's pending-slot drop path to use `Option::map` (clippy::manual_map) — cosmetic, no behavior change.
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
github-actions
Bot
added
size: L
![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: bf12ce9524
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+423
to
+428
| if pending_after_stop { | ||
| // We already stopped on a prior suspension and | ||
| // queued no work for the inner port past that | ||
| // point. A trailing Pending slot has no outcome | ||
| // to surface; drop it. | ||
| None |
There was a problem hiding this comment.
When stop_on_first_suspension is true, this merge logic only suppresses later Pending slots but still emits later Resolved slots. In a mixed batch where an earlier hook-allowed call suspends in the inner port and a later invocation was already preflight-resolved (for example Denied), the later outcome is still returned even though sequential-stop semantics should truncate the batch at the first suspension. This diverges from the sequential behavior used by other batch ports and can change loop behavior (e.g., GateOutcome::SkipAndContinue can cause those post-suspension outcomes to be processed). After the first suspension is observed, remaining slots should be dropped uniformly (except the suspension slot itself, whose observer should still fire).
Useful? React with 👍 / 👎.
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…nearai#3912/nearai#3913 (nearai#3921) * refactor(hooks): align ExtensionId/HookLocalId with newtype template Address henrypark133 approval-with-followup items from PR nearai#3912: - L1: remove dead empty-id guard in HookManifestEntry::validate. HookLocalId::new now rejects empty strings at construction, so manifest deserialization fails before validate() is ever called. - L2: add AsRef<str>, From<Self> for String, and into_inner() to ExtensionId and HookLocalId per the canonical newtype template in .claude/rules/types.md. into_string() is retained as a thin alias for compatibility; new code should prefer into_inner(). - L3: document why the builtin_id_distinct_from_extension_id test fixture substitutes "path.module" for the original "path::module" (the new grammar rejects colons in HookLocalId). No behavioral change. * fix(hooks): fire AfterCapability observer for hook-suspended entries after allowed entries henrypark133 M1 on PR nearai#3911: in `HookedLoopCapabilityPort::invoke_capability_batch`, when Phase 1 produced a mix of Pending (hook-allowed) and Resolved (hook-suspension) slots with the suspension appearing AFTER an allowed entry and `stop_on_first_suspension = true`, the merge loop initialized `stopped_on_suspension` from `stopped_in_preflight` and broke after the very first iteration. Result: trailing Resolved suspension slots never fired their `AfterCapability` observer and never surfaced in the merged `outcomes` vec, violating the per-entry observer contract from PR nearai#3573 (serrrfirat P2 #3). Fix: continue iterating the merge loop so every slot fires its observer and every Resolved outcome is pushed. Only Pending slots are dropped after a stop (their inner work was already short-circuited in Phase 1 or by an early inner-port stop), tracked via `pending_after_stop`. Re-pop guard on `inner_outcomes.pop()` keeps the previous "inner stopped early on its own suspension" semantics: pending slots without an inner outcome are dropped, but the loop continues so any trailing Resolved observers still fire. New regression test `batch_invocation_fires_observer_for_hook_suspended_entry_after_allowed_entry_with_stop_on_first_suspension` pins the behavior: `[alpha=hook-allowed, beta=hook-suspension]` with `stop_on_first_suspension = true` produces a 2-entry `outcomes` vec (Completed alpha + ApprovalRequired beta), fires the observer twice, and only sends alpha to the inner port. Verified TDD-style: the test fails on the pre-fix code with `outcomes.len() = 1`. * perf(hooks): reuse serialized argument bytes in lazy resolve henrypark133 L1 on PR nearai#3913: `resolve_arguments` measured the post-resolver JSON payload by calling `serde_json::to_vec(&value)` and discarding the `Vec<u8>` once its length was checked. The `SanitizedArguments::from_json` constructor on the happy path sanitizes the in-memory `serde_json::Value` directly without re-serializing, so the materialized buffer was pure overhead. Switch the size measurement to `serialized_len`, a counting `io::Write` adapter that streams `serde_json::to_writer` into a u64 counter — saves one Vec<u8> allocation and the matching drop per resolved invocation. Behavior is identical: the same JSON encoding rules drive both writers, the cap check still fires when the encoded length exceeds `MAX_PREDICATE_INPUT_BYTES`, and serialization errors still fail closed. No new tests required; existing `dispatch_fails_closed_when_input_exceeds_max_bytes` and the ordering / lazy-probe tests exercise this path. * test(hooks): cover mixed needs_input hook binding short-circuit henrypark133 L2 on PR nearai#3913: add `before_capability_needs_input_returns_true_when_any_active_binding_needs_input`. Installs two BeforeCapability bindings on the same scope (Global) — one `needs_input() = false`, one `needs_input() = true` — and asserts both ends of the short-circuit: 1. `HookDispatcher::before_capability_needs_input(None)` returns true. 2. Driving `HookedLoopCapabilityPort::invoke_capability` with an instrumented `ProbingResolver` confirms the resolver IS consulted exactly once — i.e. the short-circuit fires through the call site, not just the helper. This pins the "any input-needing binding wins" contract end-to-end so a future change to the dispatcher's probe (or to the middleware's lazy-probe gate) can't silently regress to short-circuiting on the first binding only. Also tightens the merge-loop's pending-slot drop path to use `Option::map` (clippy::manual_map) — cosmetic, no behavior change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidates henrypark133 approval-with-followup feedback from three merged hooks PRs. Each item is in its own commit; the only behavioral change is the M1 observer-loss fix on #3911.
#3911 (merged)
M1 — fix(hooks): fire AfterCapability observer for hook-suspended entries after allowed entries. In
HookedLoopCapabilityPort::invoke_capability_batch, the merge loop initializedstopped_on_suspensionfromstopped_in_preflightand broke after the first iteration. When the batch was[A=Pending(hook-allowed), B=Resolved(hook-suspension)]withstop_on_first_suspension = true, slot B'sAfterCapabilityobserver never fired and B never surfaced inoutcomes— violating the per-entry observer contract from PR feat(reborn): add ironclaw_hooks framework foundation (#3524) #3573 (serrrfirat P2 Onboarding: show Telegram in channel selection and auto-install bundled channel #3). Fix: keep walking the slot vec so every observer fires and every Resolved outcome is pushed; onlyPendingslots are dropped after a stop. (Commit3a077e9.)M2 — test: new regression
batch_invocation_fires_observer_for_hook_suspended_entry_after_allowed_entry_with_stop_on_first_suspensionverifies the fix and the mixed-batch contract end-to-end. Written TDD: confirmed the test fails on the pre-fix code (outcomes.len() == 1) before the merge-loop change was applied. (Commit3a077e9.)#3912 (merged) — commit
471cf0fif self.id.as_str().is_empty()guard inHookManifestEntry::validate—HookLocalId::newnow rejects empty strings at construction, so manifest deserialization fails beforevalidate()runs.impl AsRef<str>andimpl From<Self> for StringtoExtensionIdandHookLocalId, plus a canonicalinto_inner()method, matching the newtype template in.claude/rules/types.md.into_string()is retained as a thin alias so no downstream callers break; new code should useinto_inner().builtin_id_distinct_from_extension_iddocumenting why the test fixture substitutes"path.module"for the original"path::module"(the post-refactor(hooks): enforce identity newtype validation at construction #3912 segment grammar disallows:inHookLocalId).#3913 (merged)
L1 — perf(hooks): reuse serialized argument bytes in lazy resolve.
resolve_argumentspreviously calledserde_json::to_vec(&value)purely to measure the payload's byte length and then dropped theVec<u8>;SanitizedArguments::from_jsondoes not re-serialize on the happy path. Replaced with a countingio::Writeadapter (serialized_len) that streamsserde_json::to_writerinto a u64 counter — saves one Vec allocation + matching drop per resolved invocation. Behavior identical: same JSON encoding rules, same cap check, same fail-closed on serialization error. (Commite6530a8.)L2 — test: new
before_capability_needs_input_returns_true_when_any_active_binding_needs_inputinstalls two BeforeCapability bindings on the same scope (oneneeds_input()=false, oneneeds_input()=true), then asserts (a) the dispatcher's probe returns true and (b) drivinginvoke_capabilitywith an instrumentedProbingResolvercausesresolveto be called exactly once — pinning the short-circuit through the call site, not just the helper. (Commitbf12ce9.)Test plan
cargo fmt --allcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo test -p ironclaw_hooks -p ironclaw_rebornironclaw_hooks: 213 passed (was 211; +1 M1 regression, +1 L2 short-circuit test)ironclaw_reborn: all greenoutcomes.len() = 1, expected 2) and passes after the fix.Notes / out of scope
SanitizedArguments::from_json_bytesconstructor that reuses the measured bytes. After tracing the code path, the existingfrom_jsonsanitizes the in-memoryserde_json::Valuedirectly (no re-serialize), so the actual perf win was avoiding the throwawayVec<u8>— implemented via the counting-writer instead, which is local tocapability_port.rsand avoids churningSanitizedArguments' API.ExtensionId::into_stringandHookLocalId::into_stringare kept as aliases. No callers currently use them, but the alias is the conservative path; a follow-up could mark them#[deprecated]or delete them outright once the rest of the workspace finishes migrating tointo_inner.