fix(hooks): enforce allowedAgentIds for implicit routing

[RichardCao]

Summary

• apply hooks.allowedAgentIds to implicit /hooks/agent routing instead of only explicit agentId overrides
• deny implicit routing when the default agent is not allowlisted
• add helper and integration coverage for implicit default-agent enforcement

Testing

• ./node_modules/.bin/vitest run src/gateway/hooks.test.ts src/gateway/server.hooks.test.ts

[greptile-apps]

Greptile Summary

This PR aims to clarify (and ostensibly enforce) how hooks.allowedAgentIds applies to implicit /hooks/agent routing — requests that omit agentId and fall through to the default agent. The only code change in hooks.ts is a comment update; the functional behaviour is identical to before. New unit and integration tests are added to document that implicit routing is always allowed regardless of the allowlist, and the existing integration test gains peekSystemEvents assertions for completeness.\n\nKey concern:\n- The PR title ("enforce allowedAgentIds for implicit routing") and description ("deny implicit routing when the default agent is not allowlisted") directly contradict the implementation. The code still returns true unconditionally when agentId is undefined, and the new test is explicitly named "keeps implicit routing compatible regardless of allowlist." A consumer that sets allowedAgentIds: [\"hooks\"] will find that implicit requests are still dispatched to the main default agent, bypassing the allowlist entirely. Either the implementation needs to enforce the allowlist against the resolved default agent for implicit calls, or the PR title/description must be corrected to reflect that implicit routing is intentionally exempt.

Confidence Score: 4/5

The code changes are internally self-consistent, but the PR's stated security intent (deny implicit routing when the default agent is not allowlisted) is not implemented, creating a potential allowlist bypass.

The test changes and comment update are sound, and mock handling in the integration tests is correct. However, there is a clear P1 discrepancy: the PR description explicitly states that implicit routing should be denied when the default agent is absent from the allowlist, yet isHookAgentAllowed still returns true unconditionally for undefined agentId. This gap should be resolved before merging.

src/gateway/hooks.ts — isHookAgentAllowed needs to either enforce the allowlist for the implicit (default-agent) path or the PR description must be updated to acknowledge the bypass.

Important Files Changed

Filename	Overview
src/gateway/hooks.ts	Comment-only change to isHookAgentAllowed; the allowlist bypass for implicit routing (undefined agentId always returns true) is unchanged, contradicting the PR description's claim that implicit routing is denied when the default agent is not allowlisted.
src/gateway/hooks.test.ts	Updated test name to 'only enforces … for explicit routing' and added a new unit test that correctly documents current behaviour (implicit routing always allowed regardless of allowlist). Internally consistent.
src/gateway/server.hooks.test.ts	Adds peekSystemEvents assertions and a new implicit-routing block to two existing allowedAgentIds tests. Mock handling (mockClear before each section) is correct, and the type refinement (adding

Comments Outside Diff (1)

1. src/gateway/hooks.ts, line 289-294 (link)

   PR description contradicts implementation

   The PR title says "enforce allowedAgentIds for implicit routing" and the description says "deny implicit routing when the default agent is not allowlisted," but the implementation is unchanged: implicit routing (undefined agentId) still returns true unconditionally — the allowlist is never consulted for the default-agent path.

   This means a caller that sets allowedAgentIds: ["hooks"] to restrict traffic to only the hooks agent will still have implicit requests dispatched to the main default agent, silently bypassing the allowlist. The new unit test even names this "keeps implicit routing compatible regardless of allowlist," which directly contradicts the stated fix.

   If the intent is truly to enforce the allowlist for implicit routing (i.e., check whether the resolved default agent ID is in the allowlist), the implementation needs to look up the default agent and check it:

   const raw = agentId?.trim();
   if (!raw) {
     // Enforce the allowlist against the default agent for implicit routing.
     const defaultId = hooksConfig.agentPolicy.defaultAgentId;
     const allowed = hooksConfig.agentPolicy.allowedAgentIds;
     if (allowed === undefined || !defaultId) {
       return true;
     }
     return allowed.has(defaultId);
   }

   If the decision is instead to keep backwards compatibility and always allow implicit routing, the PR title/description and the "Testing" section should be updated to reflect that, so reviewers and future developers aren't misled about the security boundary.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: src/gateway/hooks.ts
   Line: 289-294
   
   Comment:
   **PR description contradicts implementation**
   
   The PR title says "enforce `allowedAgentIds` for implicit routing" and the description says "deny implicit routing when the default agent is not allowlisted," but the implementation is unchanged: implicit routing (undefined `agentId`) still returns `true` unconditionally — the allowlist is never consulted for the default-agent path.
   
   This means a caller that sets `allowedAgentIds: ["hooks"]` to restrict traffic to only the `hooks` agent will still have implicit requests dispatched to the `main` default agent, silently bypassing the allowlist. The new unit test even names this "keeps implicit routing compatible **regardless of allowlist**," which directly contradicts the stated fix.
   
   If the intent is truly to enforce the allowlist for implicit routing (i.e., check whether the resolved default agent ID is in the allowlist), the implementation needs to look up the default agent and check it:
   
   ```ts
   const raw = agentId?.trim();
   if (!raw) {
     // Enforce the allowlist against the default agent for implicit routing.
     const defaultId = hooksConfig.agentPolicy.defaultAgentId;
     const allowed = hooksConfig.agentPolicy.allowedAgentIds;
     if (allowed === undefined || !defaultId) {
       return true;
     }
     return allowed.has(defaultId);
   }
   ```
   
   If the decision is instead to keep backwards compatibility and always allow implicit routing, the PR title/description and the "Testing" section should be updated to reflect that, so reviewers and future developers aren't misled about the security boundary.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/gateway/hooks.ts
Line: 289-294

Comment:
**PR description contradicts implementation**

The PR title says "enforce `allowedAgentIds` for implicit routing" and the description says "deny implicit routing when the default agent is not allowlisted," but the implementation is unchanged: implicit routing (undefined `agentId`) still returns `true` unconditionally — the allowlist is never consulted for the default-agent path.

This means a caller that sets `allowedAgentIds: ["hooks"]` to restrict traffic to only the `hooks` agent will still have implicit requests dispatched to the `main` default agent, silently bypassing the allowlist. The new unit test even names this "keeps implicit routing compatible **regardless of allowlist**," which directly contradicts the stated fix.

If the intent is truly to enforce the allowlist for implicit routing (i.e., check whether the resolved default agent ID is in the allowlist), the implementation needs to look up the default agent and check it:

```ts
const raw = agentId?.trim();
if (!raw) {
  // Enforce the allowlist against the default agent for implicit routing.
  const defaultId = hooksConfig.agentPolicy.defaultAgentId;
  const allowed = hooksConfig.agentPolicy.allowedAgentIds;
  if (allowed === undefined || !defaultId) {
    return true;
  }
  return allowed.has(defaultId);
}
```

If the decision is instead to keep backwards compatibility and always allow implicit routing, the PR title/description and the "Testing" section should be updated to reflect that, so reviewers and future developers aren't misled about the security boundary.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (2): Last reviewed commit: "fix(hooks): preserve implicit default ro..." | Re-trigger Greptile}

[RichardCao]

Rebased this PR onto the latest main because the previous failures were coming from unrelated baseline drift in compaction/skills/plugin-sdk, not from the hooks change itself.

After rebasing, I re-ran the relevant checks locally:

• pnpm build
• ./node_modules/.bin/vitest run src/gateway/hooks.test.ts src/gateway/server.hooks.test.ts

Both pass on the updated branch. The hooks fix itself is unchanged in behavior; this update only carries the branch onto a green base.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 005dfd0d12

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Avoid blocking mapped hooks without explicit agentId

isHookAgentAllowed now denies any call where agentId is omitted unless the default agent is in hooks.allowedAgentIds, but this helper is also used by mapped webhook actions in src/gateway/server-http.ts (the mapped action path calls the same check). As a result, mappings that intentionally omit agentId (including preset-style mappings that rely on default routing) will now return 400 when the allowlist excludes the default agent, which is a behavior regression from prior releases and can silently break existing webhook automations after upgrade.

Useful? React with 👍 / 👎.

[RichardCao]

Accepted. The review was correct: hooks.allowedAgentIds is documented and audited as a control for explicit agent routing, so my previous change tightened semantics too far and broke implicit/mapped default routing.\n\nUpdated in f52080b to preserve backwards-compatible implicit routing when agentId is omitted, while still enforcing the allowlist for explicit agentId requests.\n\nLocal validation:\n- pnpm exec vitest run src/gateway/hooks.test.ts src/gateway/server.hooks.test.ts\n- 31 tests passed

[steipete]

Closing this as not actionable after Codex review.

PR #55607 is internally contradictory: its title/body ask to enforce hooks.allowedAgentIds for implicit routing, but current OpenClaw docs/code/tests define that setting as an explicit-routing control, and the PR’s later review history switched to preserving implicit default routing instead.

What I checked:

• Docs define allowedAgentIds as explicit-routing only: The Gateway configuration reference says allowedAgentIds "restricts explicit routing" and documents */omitted/[] semantics accordingly. Public docs: docs/gateway/configuration-reference.md. (docs/gateway/configuration-reference.md:481, d3d9b5738f83)
• Current helper explicitly allows omitted agentId: isHookAgentAllowed returns true when agentId is omitted, with a backwards-compatibility comment, so current main does not implement the PR title/body’s stricter implicit-routing behavior. (src/gateway/hooks.ts:300, d3d9b5738f83)
• Same helper gates both direct and mapped hook routing: server-http.ts checks isHookAgentAllowed(...) for /hooks/agent requests and for mapped hook actions, so changing implicit handling would also alter mapping behavior. (src/gateway/server-http.ts:669, d3d9b5738f83)
• Tests on main cover explicit-only enforcement: Gateway tests assert that omitted agentId is allowed while explicit agentId values are constrained by hooks.allowedAgentIds, matching docs and implementation. (src/gateway/server.hooks.test.ts:671, d3d9b5738f83)
• PR history no longer matches its own title/body: The supplied PR context shows the title/body still say implicit routing should be denied, but the later review thread and head commit f52080bcc12e4356efb89f12653f1381b577ecd2 explicitly switched to preserving implicit default routing. That leaves no single coherent change request to merge. (f52080bcc12e)

Review notes: reviewed against d3d9b5738f83.