Greptile Summary

This PR repairs the shared-hook announcement policy from #55761: successful deliver:false hook runs are now silenced (with an audit log entry), non-ok results (error/skipped) still surface as system events, and hooks whose delivery was already attempted (deliveryAttempted: true) correctly suppress the fallback announcement. Target-agent session routing from #73228 is preserved throughout.

The shouldAnnounceHookRunResult helper centralises the policy in one testable function, the new test cases cover all four code paths (silent ok, announced error, announced skipped, deliveryAttempted suppression), and the sourcePath field is correctly threaded through both the direct and mapped dispatch sites in hooks-request-handler.ts.

Confidence Score: 5/5

This PR is safe to merge — the announcement policy logic is correct, well-tested, and limited in scope.

No P0 or P1 issues found. The shouldAnnounceHookRunResult predicate correctly handles all status × deliver × delivered × deliveryAttempted combinations. New tests cover every path added. The sourcePath field is threaded through all dispatch call sites and the type system would surface any missed callers at compile time.

No files require special attention.

_{Reviews (5): Last reviewed commit: "fix(hooks): audit suppressed hook succes..." | Re-trigger Greptile}

Codex review: needs maintainer review before merge.

What this changes:

This PR adds a Gateway hook announcement predicate, sourcePath audit metadata, regression tests for silent deliver:false, non-ok surfacing, and deliveryAttempted suppression, plus a changelog entry.

Maintainer follow-up before merge:

Keep this PR open for maintainer review. If accepted, land the narrow Gateway hook announcement-policy change after resolving or documenting the raw sessionKey audit-log question and running the stated validation list; keep announceToMain and prefix-cleanup work as separate follow-ups.

Best possible solution:

Keep this PR open for maintainer review. If accepted, land the narrow Gateway hook announcement-policy change after resolving or documenting the raw sessionKey audit-log question and running the stated validation list; keep announceToMain and prefix-cleanup work as separate follow-ups.

Acceptance criteria:

• pnpm test src/gateway/server/hooks.agent-trust.test.ts
• pnpm test src/gateway/server.hooks.test.ts -t "hook announcement policy"
• pnpm test src/gateway/server.hooks.test.ts -t "target agent"
• pnpm test src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts
• pnpm check:changed

What I checked:

• Current main fallback remains coarse: dispatchAgentHook still enqueues a system event whenever !result.delivered; it does not consult value.deliver, result.status, or result.deliveryAttempted. (src/gateway/server/hooks.ts:104, 39f810911c38)
• Current main test expects noisy deliver:false success: The current hook trust test builds a payload with deliver:false, mocks an ok result with delivered:false, and expects enqueueSystemEvent to receive Hook ...: done. (src/gateway/server/hooks.agent-trust.test.ts:89, 39f810911c38)
• Typed result contract supports the proposed policy: RunCronAgentTurnResult exposes delivered?, deliveryAttempted?, and the CronRunOutcome status, so the hook layer has enough typed data to distinguish silent successes, non-ok outcomes, and already-attempted delivery paths. (src/cron/isolated-agent/run.types.ts:3, 39f810911c38)
• Public hook surface: The public automation docs document POST /hooks/agent and list deliver among its fields, so the behavior is user-visible Gateway hook behavior rather than internal-only cleanup. Public docs: docs/automation/cron-jobs.md. (docs/automation/cron-jobs.md:255, 39f810911c38)
• PR patch targets the missing behavior: The PR diff adds shouldAnnounceHookRunResult, switches the fallback gate from !result.delivered to that predicate, threads sourcePath, and adds tests for direct/mapped deliver:false silence, non-ok surfacing, and deliveryAttempted suppression. (src/gateway/server/hooks.ts:25, 7199f05a6564)
• Security review: new raw sessionKey audit metadata: The PR adds an info audit log for suppressed successful hooks containing sessionKey; current file logging maps sessionKey into session_id, and default redaction patterns do not generally redact session routing identifiers. This matches the Aisle low-severity review concern and needs maintainer signoff or minimization before merge. (src/gateway/server/hooks.ts:124, 7199f05a6564)

Likely related people:

• vincentkoc: Merged and authored the adjacent target-agent hook routing fix, is assigned in the provided timeline, force-pushed the current branch, and authored the PR's audit-log follow-up commit. (role: recent maintainer / adjacent owner; confidence: high; commits: 6f38425e5c18, a276ca43d871, 7199f05a6564; files: src/gateway/server/hooks.ts, src/gateway/server.hooks.test.ts, src/gateway/server/hooks.agent-trust.test.ts)
• JonathanWorks: The current !result.delivered hook fallback guard traces to the merged hook/cron fix for suppressing duplicate main-session events on delivered or silent hook turns. (role: introduced related current-main fallback behavior; confidence: medium; commits: 8c089bbe3235; files: src/gateway/server/hooks.ts, src/cron/isolated-agent/run.ts, CHANGELOG.md)
• EffortlessSteven: The PR body and discussion identify fix(hooks): suppress silent shared-hook fallback while preserving error surfacing #55761 by EffortlessSteven as the broader source implementation, and the discussion confirms this PR as the narrower landing path while keeping remaining hook-policy pieces as follow-ups. (role: source implementation author; confidence: medium; commits: b84a19d3dfbb, 52cd4bd369a8; files: src/gateway/server/hooks.ts, src/gateway/server.hooks.test.ts, src/gateway/server/hooks.agent-trust.test.ts)
• BrennerSpear: Authored the earlier fix(hooks): suppress system event injection when deliver is false #49234 iteration that added deliver:false suppression coverage and built on the original fix(hooks): suppress system event injection when deliver is false [AI-assisted] #36332 fix, which this PR explicitly credits. (role: prior implementation/test contributor; confidence: low; commits: b0edd799679c; files: src/gateway/server.hooks.test.ts)

Remaining risk / open question:

• The PR adds an info-level audit log containing raw sessionKey; session routing identifiers can include peer/channel/account context, and current default redaction does not clearly minimize those identifiers.
• The PR intentionally leaves broader fix(hooks): suppress silent shared-hook fallback while preserving error surfacing #55761 pieces such as result-level announceToMain and Hook Hook prefix cleanup for follow-up, so maintainers should confirm the narrow scope is the desired landing path.
• The PR body still asks for targeted hook tests, pnpm check:changed, pnpm build, and parity-gate investigation before merge; this read-only review did not run those gates.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 39f810911c38.

Thanks for opening the replacement.

Happy with this as the narrow landing path if that's the maintainer preference. I can take the remaining hook-policy pieces as follow-ups, or keep maintaining the broader source implementation, whichever is more useful.

For context: I'd been keeping #55761 current with main through both the hooks.ts lazy-import collision and #73228's target-agent completion/error routing. After the final rebase, it was locally validated in a clean detached worktree. It preserved:

• security(gateway): route hook completion events to target agent session #73228's target-agent routing
• the hook announcement-policy gate
• Hook Hook prefix cleanup
• explicit-agent announceToMain composition coverage

It passed targeted hook tests, pnpm check:changed, and pnpm build.

This PR looks like a clean narrow path for the core behavior:

• successful deliver:false hooks stay silent
• non-ok hook results still surface
• handled deliveryAttempted paths don't duplicate fallback events
• target-agent routing stays composed with the fallback policy

The main delta from #55761 is that this leaves out:

• result-level announceToMain?: boolean
• formatHookPrefix(...) / Hook Hook cleanup in success and catch paths
• broader explicit override / NO_REPLY policy coverage

If this lands as-is, I'd treat those as follow-up hook-policy work.

On the red parity check: the failure was a timeout in thread-memory-isolation. I didn't see /hooks/agent, enqueueSystemEvent, agent:hooks:main, announceToMain, deliveryAttempted, or hook fallback references in the artifact.

Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit.

Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit.

Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit.

Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit.

Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit.

Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit.