Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds agent-inactive gating: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Webhook Client
participant Controller as AgentsWebhookController
participant Resolver as AgentConfigResolver
participant Repo as AgentRepository
Client->>Controller: POST /v1/agents/:id/webhook
Controller->>Resolver: resolve(agentId)
Resolver->>Repo: findByIdForWebhook(agentId)
Repo-->>Resolver: agent { active: false }
Resolver-->>Controller: throw AgentInactiveException(agentId)
Controller-->>Client: HTTP 200 OK + {}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/api/src/app/agents/e2e/agent-webhook.e2e.ts (1)
293-301: Reactivation test bypasses the HTTP webhook path.
invokeInboundcallsconfigResolver.resolve(...)+inboundHandler.handle(...)directly, so this test doesn't actually exercise thePOST /v1/agents/:agentId/webhook/...controller after reactivation — which is the path the webhook behavior change applies to. The "inactive returns 200" test above does go through HTTP; consider mirroring that here (sign and POST a Slack app_mention after the reactivation PATCH) so the full controller + exception-handling path is validated end-to-end on reactivation as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/e2e/agent-webhook.e2e.ts` around lines 293 - 301, The test currently calls invokeInbound directly (which resolves config and calls inboundHandler.handle) so it bypasses the HTTP controller path; replace the direct invokeInbound call in the 'should process inbound again after reactivation' test with an actual signed POST to the webhook endpoint (POST /v1/agents/:agentId/webhook/...) using the same signing helper used in the "inactive returns 200" test and send a Slack app_mention payload (e.g., mockMessage with userId 'U_REACTIVATED'), so the request goes through the controller, middleware, and exception handling just like real webhook traffic; keep the existing reactivation PATCH calls and assert bridgeCalls.length === 1 after the POST.apps/api/src/app/agents/e2e/agent-reply.e2e.ts (1)
320-333: Assert the PATCH succeeded before posting the reply.If the
PATCH /v1/agents/:agentIdentifiercall silently fails (e.g., auth/route regression), the agent would remainactiveandpostReplywould return 200, making the test pass spuriously until the actual 422 assertion line runs. A small guard likeexpect(patchRes.status).to.equal(200)on the PATCH response would make the failure mode obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/e2e/agent-reply.e2e.ts` around lines 320 - 333, The test doesn't assert the PATCH succeeded before calling postReply; capture the PATCH response (e.g., const patchRes = await ctx.session.testAgent.patch(`/v1/agents/${ctx.agentIdentifier}`).send({ active: false })) and assert its status is 200 (expect(patchRes.status).to.equal(200)) before calling postReply so the test fails clearly if the agent deactivation did not actually occur; keep the existing postReply/assertion unchanged.apps/dashboard/src/components/agents/agent-overview-tab.tsx (1)
22-32: Considerrole="status"for the deactivation banner.The banner communicates a non-transient operational state; adding
role="status"(orrole="alert"if you want it announced immediately on deactivation) would make the warning discoverable to screen readers rather than being just a visual affordance. UsingRiErrorWarningLinewitharia-hiddenonly leaves the message text as an unannounced span.a11y tweak
- <div className="border-b border-warning-base/20 bg-warning-lighter px-6 py-2.5"> + <div role="status" className="border-b border-warning-base/20 bg-warning-lighter px-6 py-2.5">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/agents/agent-overview-tab.tsx` around lines 22 - 32, The deactivation banner in agent-overview-tab.tsx is only visual and not announced to screen readers; add an appropriate ARIA role to the outer banner div (the <div className="border-b border-warning-base/20 bg-warning-lighter px-6 py-2.5"> block) — use role="status" to make the non-transient state discoverable (or role="alert" if you need immediate announcement on change) and keep the icon aria-hidden so the <span> text is announced; ensure the role is applied to the same element that contains the message text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/src/app/agents/e2e/agent-reply.e2e.ts`:
- Around line 320-333: The test doesn't assert the PATCH succeeded before
calling postReply; capture the PATCH response (e.g., const patchRes = await
ctx.session.testAgent.patch(`/v1/agents/${ctx.agentIdentifier}`).send({ active:
false })) and assert its status is 200 (expect(patchRes.status).to.equal(200))
before calling postReply so the test fails clearly if the agent deactivation did
not actually occur; keep the existing postReply/assertion unchanged.
In `@apps/api/src/app/agents/e2e/agent-webhook.e2e.ts`:
- Around line 293-301: The test currently calls invokeInbound directly (which
resolves config and calls inboundHandler.handle) so it bypasses the HTTP
controller path; replace the direct invokeInbound call in the 'should process
inbound again after reactivation' test with an actual signed POST to the webhook
endpoint (POST /v1/agents/:agentId/webhook/...) using the same signing helper
used in the "inactive returns 200" test and send a Slack app_mention payload
(e.g., mockMessage with userId 'U_REACTIVATED'), so the request goes through the
controller, middleware, and exception handling just like real webhook traffic;
keep the existing reactivation PATCH calls and assert bridgeCalls.length === 1
after the POST.
In `@apps/dashboard/src/components/agents/agent-overview-tab.tsx`:
- Around line 22-32: The deactivation banner in agent-overview-tab.tsx is only
visual and not announced to screen readers; add an appropriate ARIA role to the
outer banner div (the <div className="border-b border-warning-base/20
bg-warning-lighter px-6 py-2.5"> block) — use role="status" to make the
non-transient state discoverable (or role="alert" if you need immediate
announcement on change) and keep the icon aria-hidden so the <span> text is
announced; ensure the role is applied to the same element that contains the
message text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1fd339e5-2f87-41d2-8c02-f75ec5ebb5a9
📒 Files selected for processing (8)
apps/api/src/app/agents/agents-webhook.controller.tsapps/api/src/app/agents/e2e/agent-reply.e2e.tsapps/api/src/app/agents/e2e/agent-webhook.e2e.tsapps/api/src/app/agents/exceptions/agent-inactive.exception.tsapps/api/src/app/agents/services/agent-config-resolver.service.tsapps/dashboard/src/components/agents/agent-overview-tab.tsxapps/dashboard/src/components/agents/agent-sidebar-widget.tsxapps/dashboard/src/components/agents/agents-table.tsx
Base automatically changed from
refactor/agent-conversation-architecture
to
next
April 20, 2026 15:42
ChmaraX
force-pushed
the
nv-7393-enforce-agent-active-toggle-gate-inbound-processing
branch
from
338cc71 to
273e01e
Compare
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/api/src/app/agents/e2e/agent-webhook.e2e.ts (1)
293-300: Exercise the actual webhook route after reactivation.This test bypasses the controller by calling
invokeInbound(), so it won’t catch regressions inAgentsWebhookController.routeWebhook()orChatSdkService.handleWebhook()after an inactive-agent short-circuit. Reuse the signed Slack POST path from the inactive test for the reactivation assertion.🧪 Proposed test adjustment
it('should process inbound again after reactivation', async () => { await ctx.session.testAgent.patch(`/v1/agents/${ctx.agentIdentifier}`).send({ active: false }); await ctx.session.testAgent.patch(`/v1/agents/${ctx.agentIdentifier}`).send({ active: true }); - const threadId = `T_REACTIVATE_${Date.now()}`; - await invokeInbound(threadId, mockMessage({ userId: 'U_REACTIVATED', text: 'Back online' })); + const body = JSON.stringify( + buildSlackAppMention({ userId: 'U_REACTIVATED', channel: 'C_TEST', threadTs: `T_REACTIVATE_${Date.now()}` }) + ); + const timestamp = Math.floor(Date.now() / 1000); + const headers = signSlackRequest(ctx.signingSecret, timestamp, body); + + const res = await ctx.session.testAgent + .post(`/v1/agents/${ctx.agentId}/webhook/${ctx.integrationIdentifier}`) + .set(headers) + .set('content-type', 'application/json') + .send(body); + expect(res.status).to.equal(200); expect(bridgeCalls.length).to.equal(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/e2e/agent-webhook.e2e.ts` around lines 293 - 300, Replace the direct helper call invokeInbound(...) with an actual signed Slack POST request that hits the controller route used in the inactive-agent test so the request flows through AgentsWebhookController.routeWebhook and ChatSdkService.handleWebhook; specifically, reuse the same signed Slack POST helper/fixture used earlier in the inactive test to post the mock message (userId 'U_REACTIVATED', text 'Back online') to the webhook endpoint after reactivation, then assert bridgeCalls.length === 1 as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/agents/e2e/agent-reply.e2e.ts`:
- Around line 320-332: The test currently only exercises the "reply" path and
misses the signals-only branch that calls AgentConfigResolver.resolve()
conditionally; add a signals-only assertion so inactive agents cannot be
bypassed by signals: in apps/api/src/app/agents/e2e/agent-reply.e2e.ts (the
describe 'Inactive agent' block) add or modify an it() case that posts a payload
with reply null/omitted and a non-empty signals array via postReply (or the same
helper used) for the inactive ctx.agentIdentifier and assert the response is
422; this ensures the handle-agent-reply usecase (see
AgentConfigResolver.resolve and AgentInactiveException in
handle-agent-reply.usecase.ts) is exercised for signals-only requests.
In `@apps/api/src/app/agents/services/agent-config-resolver.service.ts`:
- Around line 81-83: The current check "if (!agent.active) { throw new
AgentInactiveException(agentId); }" treats undefined as inactive; change it to
explicitly check for a false boolean by replacing the condition with "if
(agent.active === false) ..." so only agents with active === false are rejected
(treat undefined/null as active), leaving the rest of the method (and the
AgentInactiveException(agentId) throw) unchanged.
---
Nitpick comments:
In `@apps/api/src/app/agents/e2e/agent-webhook.e2e.ts`:
- Around line 293-300: Replace the direct helper call invokeInbound(...) with an
actual signed Slack POST request that hits the controller route used in the
inactive-agent test so the request flows through
AgentsWebhookController.routeWebhook and ChatSdkService.handleWebhook;
specifically, reuse the same signed Slack POST helper/fixture used earlier in
the inactive test to post the mock message (userId 'U_REACTIVATED', text 'Back
online') to the webhook endpoint after reactivation, then assert
bridgeCalls.length === 1 as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f8b3048-c395-473d-82b1-565b1a492af5
📒 Files selected for processing (7)
apps/api/src/app/agents/agents-webhook.controller.tsapps/api/src/app/agents/e2e/agent-reply.e2e.tsapps/api/src/app/agents/e2e/agent-webhook.e2e.tsapps/api/src/app/agents/exceptions/agent-inactive.exception.tsapps/api/src/app/agents/services/agent-config-resolver.service.tsapps/dashboard/src/components/agents/agent-sidebar-widget.tsxapps/dashboard/src/components/agents/agents-table.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api/src/app/agents/exceptions/agent-inactive.exception.ts
- apps/dashboard/src/components/agents/agents-table.tsx
- apps/dashboard/src/components/agents/agent-sidebar-widget.tsx
- AgentConfigResolver throws AgentInactiveException when agent.active is false - Webhook controller catches AgentInactiveException and returns 200 OK to prevent platform retries - Reply endpoint returns 422 when agent is inactive - AgentSidebarWidget shows ConfirmationModal when toggling active → inactive - AgentsTable shows a dedicated Status column consistent with the workflow list - E2E tests: inactive webhook returns 200 with no bridge call; inactive reply returns 422 Made-with: Cursor
ChmaraX
force-pushed
the
nv-7393-enforce-agent-active-toggle-gate-inbound-processing
branch
from
273e01e to
6263b8e
Compare
ChmaraX
deleted the
nv-7393-enforce-agent-active-toggle-gate-inbound-processing
branch
12 tasks
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.
Summary
AgentConfigResolvernow throwsAgentInactiveExceptionwhenagent.activeis false — inbound webhook returns200 OK(prevents platform retry loops); reply endpoint returns422How it works
UI changes
Agent list — Status column (matches workflow list):
Agent detail — deactivation modal on toggle:
Test plan
agent-webhook.e2e.tsandagent-reply.e2e.tsMade with Cursor
What changed
This PR enforces the agent "active" toggle across inbound and reply flows so deactivated agents no longer process messages. The backend now prevents processing by throwing an AgentInactiveException when agent.active is false; inbound webhooks return 200 OK (to avoid platform retry loops) and bridge replies return 422. The dashboard adds a deactivation confirmation modal and a Status column/badge so users see and confirm inactive agents.
Affected areas
api
dashboard
Key technical decisions
Testing
Added e2e tests: inactive webhook returns 200 with zero bridge calls, and inactive reply returns 422; tests include reactivation to confirm normal processing resumes. Manual UI checks for modal, status badge, and list updates are included in the PR test plan.