fix(hooks): suppress silent shared-hook fallback while preserving error surfacing#55761
fix(hooks): suppress silent shared-hook fallback while preserving error surfacing#55761EffortlessSteven wants to merge 8 commits into
Conversation
7dfcde5 to
27b1e44
Compare
Greptile SummaryThis PR replaces the coarse Key changes:
Confidence Score: 5/5Safe to merge — the logic is correct, all branches are tested, and no regressions were introduced. No P0 or P1 issues found. The priority ordering in shouldAnnounceHookResultToMain is correct and intentional, tests cover every documented branch including the error-overrides-announceToMain:false edge case, and the formatHookPrefix fix is sound for all realistic name inputs. Prior review concerns were already addressed or are explicitly out of scope for this PR. No files require special attention.
|
| Filename | Overview |
|---|---|
| src/gateway/server/hooks.ts | Replaces the coarse !result.delivered fallback gate with shouldAnnounceHookResultToMain and fixes the Hook Hook prefix label via formatHookPrefix. Logic is sound and priority ordering is correct. |
| src/cron/isolated-agent/run.ts | Adds the optional announceToMain?: boolean field to RunCronAgentTurnResult. Type-only addition, backward-compatible, well-documented. |
| src/gateway/server.hooks.test.ts | Adds 8 new targeted tests for the announcement policy using a deterministic flushHookDispatch helper. New describe block has its own afterEach with drain + restore. Tests cover all documented branches of shouldAnnounceHookResultToMain. |
Reviews (2): Last reviewed commit: "Merge branch 'main' into fix/hooks-annou..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
27b1e44 to
e1af4f8
Compare
|
@greptileai please verify that the catch-path label fix, the broadened formatHookPrefix behavior, and the afterEach event drain address the prior review comments. |
![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: d88c95c952
ℹ️ 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".
| return false; | ||
| } | ||
| // - delivery-attempted hooks should not fallback into main | ||
| if (result.deliveryAttempted === true) { |
There was a problem hiding this comment.
deliveryAttempted as a hard no-announce signal
shouldAnnounceHookResultToMain now suppresses main-session fallback whenever result.deliveryAttempted === true, but that flag is also set in non-delivery paths (for example in dispatchCronDelivery when descendant orchestration is still in progress or stale interim text is being suppressed, where no user-visible delivery has actually happened). In those status: "ok" cases, this change drops the only fallback system event for shared hooks, so hook runs can become silently invisible in the main session. This regression is introduced by the new guard and is reproducible for hook flows that return delivered: false, deliveryAttempted: true without an actual outbound send.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I’m keeping the deliveryAttempted guard here.
In src/cron/isolated-agent/delivery-dispatch.ts, deliveryAttempted is already used as a handled/no-fallback signal in non-send status: "ok" paths, not just for outbound-send attempts. The active-descendant orchestration path and the stale-interim suppression path both set deliveryAttempted = true specifically so the timer does not fire redundant enqueueSystemEvent fallback, and the stale-delivery skip paths follow the same contract.
So for shared hooks, treating deliveryAttempted as “do not fallback into main” is intentional and consistent with the existing delivery layer semantics. I clarified that contract in the helper comment and tightened the matching test wording in this PR rather than changing behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bc9d7e1af
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bc9d7e1af
ℹ️ 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".
6bc9d7e to
4cb0826
Compare
c60c28d to
e91c3f8
Compare
|
Refreshed this branch onto current Kept the shared-hook fallback behavior this PR is meant to land:
One small follow-up was needed after the rebase: Verification on the refreshed branch:
|
92215da to
00b8e3f
Compare
|
Refreshed this PR again on top of current This rebase kept the intended shared-hook fallback fix while absorbing the latest upstream hook-file shape:
Current verification on the rebased branch:
GitHub now shows the PR as mergeable again after the rebase. |
ed894cf to
f573b4b
Compare
05356ef to
b78bb80
Compare
b78bb80 to
86134fc
Compare
|
Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve. Current main still does not implement this PR's hook announcement policy. The gateway hook dispatcher still falls back to a main-session system event on Best possible solution: Keep this PR open as the active implementation candidate. The best path is to review the announcement policy against the existing cron What I checked:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against dc541662f815. |
|
Refreshed this onto current Latest pushed head: Local validation in a clean detached worktree:
No intended design change in the rebase-alignment commit. |
…or surfacing The shared-hook fallback in dispatchAgentHook previously keyed off !result.delivered alone to decide whether to inject a system event into the main session. This conflates outward delivery state with main-session announcement policy, causing silent/no-op hooks (deliver:false) and delivery-attempted hooks to leak fallback summaries into main. Replace the single-boolean gate with a compatibility-bridge helper (shouldAnnounceHookResultToMain) that respects: - result.status !== "ok" → always surface errors (even if announceToMain is false — errors override explicit suppression) - result.announceToMain (explicit policy, forward-compatible) - value.deliver === false → silent on success - result.delivered → no duplicate - result.deliveryAttempted → no fallback for async-ack paths Also fix the generic fallback label (both success and catch paths) so default name "Hook" no longer renders as "Hook Hook".
86134fc to
b84a19d
Compare
|
Refreshed again onto current Latest pushed head: The resulting composition is:
Local validation in a clean detached worktree:
|
|
Awesome. Happy to pull together the follow-up if there's interest. |
2 participants
Fixes #55704
Summary
Shared hook fallback was using the wrong signal.
The old gateway behavior treated
!result.deliveredas enough reason to post a hook summary into the resolved main-session destination. That is too coarse. A successful silent hook, a handled async delivery path, and a real hook error are different outcomes. They need different fallback behavior.This PR replaces that one-bit check with an explicit announcement policy.
The new policy:
result.status !== "ok"-> surface the hook result.announceToMain-> honor it.deliver:false-> stay silent.result.delivered === true-> avoid duplicate fallback.result.deliveryAttempted === true-> avoid handled-path fallback.It also fixes the fallback label so default or already-prefixed hook names do not render as
Hook Hook.Current main integration
This branch has been refreshed onto current
main, including #73228's target-agent hook completion/error routing.That means the final behavior is split cleanly:
shouldAnnounceHookResultToMain(...)decides whether a hook result should surface.resolveHookEventSessionKey(...)decides which resolved main-session destination receives it.For explicit-agent hooks, that destination may be the target agent's main session rather than the default main session.
What changed
Production code
shouldAnnounceHookResultToMain(...)insrc/gateway/server/hooks.ts.if (!result.delivered)fallback gate with the ordered policy above.formatHookPrefix(...)and use it in both success and catch paths.announceToMain?: booleantoRunCronAgentTurnResult.announceToMainwording to refer to the hook's resolved main-session destination.main's lazyrunCronIsolatedAgentTurnimport shape.main's target-agent hook event routing.Tests
Added or updated coverage for:
/hooks/agent+deliver:falsesuccess -> no fallbackdeliver:falsesuccess -> no fallbackdeliver:false+ non-throwstatus:"error"-> still surfacesdeliveryAttempted:true+delivered:false-> no handled-path fallbackannounceToMain:true-> fallback still works when requestedannounceToMain:false+ narration/trailingNO_REPLY-> suppressedannounceToMain:false+status:"error"-> error still surfacesHook: ..., notHook Hook: ...Hook (error): ..., notHook Hook (error): ...announceToMain:true-> routes to the target agent main session, not default mainReviewer map
Suggested path:
src/gateway/server/hooks.tsCheck the policy order, prefix formatting, and composition with
resolveHookEventSessionKey(...).src/cron/isolated-agent/run.types.tsCheck that
announceToMain?: booleanis additive and that the wording matches resolved-destination routing.src/gateway/server.hooks.test.tsCheck the announcement-policy matrix and explicit-agent routing composition case.
src/gateway/server/hooks.agent-trust.test.tsCheck that surfaced hook events stay
trusted:falseand explicit-agent events route to the target agent main session.src/gateway/test-helpers.runtime-state.tsCheck that test helper typing uses
RunCronAgentTurnResultwithout importing runtime code.Validation
Validated locally in a clean detached worktree at:
b84a19d3dfbb81cdf9028df42ca9b7fb8ecb03d2git diff --check: passoxfmt: passpnpm test src/gateway/server/hooks.agent-trust.test.ts: pass, 4 testspnpm test src/gateway/server.hooks.test.ts -t "hook announcement policy": pass, 10 testspnpm test src/gateway/server.hooks.test.ts -t "target agent": pass, 2 testspnpm test src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts: pass, 48 testspnpm check:changed: passpnpm build: pass[INEFFECTIVE_DYNAMIC_IMPORT]: not presentNote
The original Windows checkout had a local Rolldown
node_moduleslock/corruption issue. I validated the exact final head in a clean detached worktree withpnpm install --frozen-lockfile; that clean validation passed.