docs(contact-center): add core service AI documentation-AGENTS.md#4728
docs(contact-center): add core service AI documentation-AGENTS.md#4728Shreyas281299 wants to merge 102 commits intowebex:task-refactorfrom
Conversation
…ECODE_RESULTS_IN_ZERO_AUDIO_LEVEL (webex#4550)
Co-authored-by: Ravi Chandra Sekhar Sarika <rsarika@cisco.com>
…nguage (webex#4585) Co-authored-by: mickelr <121160648+mickelr@users.noreply.github.com>
Co-authored-by: vicwan <vicwan@cisco.com>
…on logic (webex#4665) Co-authored-by: vicwan <vicwan@cisco.com> Co-authored-by: Kesava Krishnan Madavan <mkesavan13@gmail.com>
…trees (webex#4697) Co-authored-by: mickelr <121160648+mickelr@users.noreply.github.com>
…ex#4637) Co-authored-by: mickelr <121160648+mickelr@users.noreply.github.com>
…#4695) Co-authored-by: Cursor <cursoragent@cursor.com>
…ity (webex#4706) Co-authored-by: James Nestor <jnestor@cisco.com> Co-authored-by: Rajesh Kumar <131742425+rarajes2@users.noreply.github.com>
…mpatibility (webex#4706)" This reverts commit ba12c61.
Shreyas281299
added
the
validated
|
@codex review |
![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: bc82f948d4
ℹ️ 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".
| export type Msg<T> = T & { | ||
| trackingId: string; | ||
| }; |
There was a problem hiding this comment.
The guide defines Msg<T> as T & { trackingId: string }, but the real Msg type in services/core/GlobalTypes.ts requires a wrapper object with type, orgId, trackingId, and data: T. This mismatch will mislead agents into generating invalid payload shapes that fail type checks and break assumptions in code paths that read msg.data and other top-level fields.
Useful? React with 👍 / 👎.
|
|
||
| ## Related | ||
|
|
||
| - [ARCHITECTURE.md](ARCHITECTURE.md) - Technical deep-dive |
There was a problem hiding this comment.
The Related section links to ARCHITECTURE.md, but there is no such file in this directory or elsewhere in the repository (checked with rg --files | rg 'ARCHITECTURE\\.md$'). Keeping this dead link reduces the usefulness of the guide because agents are directed to documentation that cannot be opened.
Useful? React with 👍 / 👎.
|
|
||
| // Usage in catch | ||
| const failure = error.details as Failure; | ||
| console.log(failure.data?.reason); |
There was a problem hiding this comment.
console.log(failure.data?.reason); — violates Critical Rule 2 . Replace with LoggerProxy.error(...)
| --- | ||
|
|
||
| ## Related | ||
|
|
There was a problem hiding this comment.
AI agents need navigation back to the orchestrator. Add link to ../../../AGENTS.md
| @@ -0,0 +1,339 @@ | |||
| # Core Service - AI Agent Guide | |||
|
|
|||
| > **Purpose**: Core infrastructure components including WebSocket management, HTTP requests, error handling, and utilities. | |||
There was a problem hiding this comment.
Should state "This is the authoritative doc for Core scope" per root AGENTS.md Rule 7
| @@ -0,0 +1,339 @@ | |||
| # Core Service - AI Agent Guide | |||
|
|
|||
| > **Purpose**: Core infrastructure components including WebSocket management, HTTP requests, error handling, and utilities. | |||
There was a problem hiding this comment.
Purpose and Overview seem to have similar agenda of basically proving a basic description for this folder so maybe we can keep just one heading and have the description there.
|
|
||
| --- | ||
|
|
||
| ## Key Components |
There was a problem hiding this comment.
In some of the files we are stating Key components and some of them has key capabilities. We should us similar headings across all the files. Also under this section when we talk about, purpose or description of that folder I feel should be more detailed
|
|
||
| --- | ||
|
|
||
| ## WebSocketManager |
There was a problem hiding this comment.
When we are talking about each of these key components, adding a definition for each of these components would be good which talks about what that file does, how it is used
|
|
||
| --- | ||
|
|
||
| ## AqmReqs |
There was a problem hiding this comment.
Context should be given here in tersm like we have routing and contact layer which using this AQM layer to send HTTP requests to contact center backend and code example should be enclosed under Reference Usage heading
|
|
||
| --- | ||
|
|
||
| ## Connection Lifecycle |
There was a problem hiding this comment.
This block should be talked closer to where we are talking about WebSocketManager and keep alive as well. And we should put some text to show the linking between each of these how they work together and wehy it was done in this manner
|
|
||
| --- | ||
|
|
||
| ## Constants |
There was a problem hiding this comment.
We don't have to put code reference for constants, we can just say that any constant to be defined for core should be in constant.ts file
- Fix Msg<T> type to match real GlobalTypes.ts definition (wrapper with type, orgId, trackingId, data fields instead of intersection type) - Fix Failure type to use Msg<T> as defined in source - Fix AugmentedError to match source (Record<string, any>) - Replace console.log with LoggerProxy.error per Critical Rule 2 - Add authoritative doc statement per root AGENTS.md Rule 7 - Add link to root orchestrator AGENTS.md for navigation - Remove dead ARCHITECTURE.md link - Merge Purpose and Overview into single section - Rename "Key Components" to "Key Capabilities" for cross-file consistency - Add detailed descriptions for each component in table - Add AqmReqs context explaining routing/contact layer usage - Move Connection Lifecycle under WebSocketManager section - Replace constants code block with reference to constants.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8af0559ebc
ℹ️ 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".
| // Align credentials against warmed catalog | ||
| this.updateCredentialsConfig(); | ||
|
|
||
| return true; |
There was a problem hiding this comment.
_loadCatalogFromCache() unconditionally returns true even when every cached group is skipped (for example, preauth cache with selectionType: mode or a selection mismatch), and initialize() treats that true as fully warmed and exits before fetching fresh catalogs. In unauthenticated/proximity flows where cache only contains preauth, this leaves the catalog effectively empty but marked ready, so subsequent service lookups can fail until another explicit refresh path runs.
Useful? React with 👍 / 👎.
|
|
||
| this.updateCredentialsConfig(); | ||
|
|
||
| return true; |
There was a problem hiding this comment.
The v2 _loadCatalogFromCache() has the same success condition bug: it returns true even if no cached group was applied (e.g., preauth cache intentionally skipped for proximity mode or mismatched selection metadata). initialize() then sets catalog.isReady = true and returns early, so fresh preauth/postauth discovery is skipped and consumers can run with missing service URLs.
Useful? React with 👍 / 👎.
| if (destAgentType === 'DN') { | ||
| return CONSULT_TRANSFER_DESTINATION_TYPE.DIALNUMBER; | ||
| } | ||
| if (destAgentType === 'EP-DN') { | ||
| return CONSULT_TRANSFER_DESTINATION_TYPE.ENTRYPOINT; |
There was a problem hiding this comment.
calculateDestType() checks for 'DN' and 'EP-DN', but this module’s own canonical participant values are lowercase 'dn' and 'EpDn' (see PARTICIPANT_TYPES). For real consult payloads this mapping branch is skipped, so the function falls through to toLowerCase() and emits invalid destination types like dn/epdn instead of dialNumber/entryPoint, which can break consult transfer/conference requests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
PR #4728 Review: docs(contact-center): add core service AI documentation-AGENTS.md
Blocking (Must Fix)
B1. Err.Details uses invalid error ID (line ~291)
The example shows 'Service.aqm.agent.login' but this ID does not exist in Err.ts:12-18. Valid IDs include 'Service.aqm.agent.stationLogin', 'Service.aqm.agent.logout', etc. An AI agent would generate code that fails TypeScript type checking.
B2. generateTaskErrorObject return type confusion (line ~217)
The doc implies this returns a TaskError interface, but the actual return type is AugmentedError (an Error with a data field) per Utils.ts:143-183. The TaskError interface has a completely different shape (error, trackingId, errorMessage, etc.). An AI agent may misuse the return value.
B3. AqmReqs example uses wrong property names (line ~153)
The req method's Req type (from core/types.ts:19-42) uses host as the property name, and notifFail has a union shape {bind, errMsg, err} | {bind, errId}. The example simplifies and changes property names, which would cause AI agents to generate invalid configuration objects.
B4. Root AGENTS.md references non-existent ARCHITECTURE.md for core (line 336)
The root AGENTS.md Service Routing Table expects a core ARCHITECTURE.md that doesn't exist. This creates a dead link in the orchestrator.
Suggestions (Should Fix)
S1. PR template checkboxes incomplete -- No Change Type checked, no certifications checked, testing scenarios empty. FedRAMP compliance requires these.
S2. getErrorDetails description misses stationLogin special handling -- When methodName === 'stationLogin', the function calls getStationLoginErrorData() to produce specialized error data (Utils.ts:106-117). This is important for agent-login error flows.
S3. Err.Message class entirely undocumented -- The Err module also exports Err.Message (Err.ts:86-110), used extensively in aqm-reqs.ts error handling. Only Err.Details is documented.
S4. 8 of 11 exported Utils.ts functions not documented -- Missing: isValidDialNumber, getStationLoginErrorData, getConsultedAgentId, getDestAgentIdForCBT, calculateDestAgentId, calculateDestType, buildConsultConferenceParamData, deriveConsultTransferDestinationType.
S5. Reconnection flow missing allowAutomatedRelogin guard -- silentRelogin() is only called if this.$config.allowAutomatedRelogin is true (cc.ts:1228-1230). Also missing the socket reconnection via ConnectionService.handleSocketClose.
S6. initWebSocket return type -- Returns WelcomeResponse (which is WelcomeEvent | Error), not a full event object. The variable name welcomeEvent is slightly misleading.
Nits
- N1. Web Worker keepalive mechanism (browser-only) not mentioned -- could mislead AI agents into thinking this works in Node.js
- N2.
AqmReqsuseswindow.setTimeout(browser-only dependency) not mentioned - N3.
types.ts(definingPending,Req,Conf,Res,CbRes,BindType,Timeout) missing from Key Components table - N4.
WebexRequest.getInstance()singleton pattern:{webex}param only needed on first init, subsequent calls can omit it
| const errDetails = createErrDetailsObject(webexRequestPayload); | ||
| // Returns Err.Details with trackingId and body | ||
| ``` | ||
|
|
There was a problem hiding this comment.
[Blocking] The error ID 'Service.aqm.agent.login' does not exist. Valid IDs in Err.ts:12-18 are: 'Service.aqm.agent.stationLogin', 'Service.aqm.agent.stationLoginFailed', 'Service.aqm.agent.stateChange', 'Service.aqm.agent.reload', 'Service.aqm.agent.logout', 'Service.reqs.generic.failure', 'Service.aqm.agent.BuddyAgentsRetrieveFailed'. Fix: change to 'Service.aqm.agent.stationLogin'.
| ### Msg\<T\> | ||
|
|
||
| Generic message wrapper used throughout the SDK. All WebSocket messages and AQM responses conform to this shape: | ||
|
|
There was a problem hiding this comment.
[Blocking] generateTaskErrorObject returns AugmentedError (which extends Error with a data? field per GlobalTypes.ts:59-61), NOT TaskError. The TaskError interface has a completely different shape (error, trackingId, errorMessage, errorType, errorData, reasonCode). An AI agent using this doc might assume the return has a .error property and write throw taskError.error instead of throw taskError.
| }, | ||
| notifFail: { | ||
| bind: {type: CC_EVENTS.FAIL, data: {type: CC_EVENTS.FAIL}}, | ||
| errId: 'Service.aqm.operation', |
There was a problem hiding this comment.
[Blocking] The AqmReqs.req example uses simplified/incorrect property names. The actual Req type (core/types.ts:19-42) uses host (not simplified names), and notifFail is a union: {bind, errMsg, err} | {bind, errId}. This would cause AI agents to generate invalid configuration objects.
|
|
||
| Error handler for task operations: | ||
|
|
||
| ```typescript |
There was a problem hiding this comment.
[Suggestion] getErrorDetails has special handling when methodName === 'stationLogin': it calls getStationLoginErrorData() to produce specialized error data with user-facing message and fieldName fields (Utils.ts:106-117). This is important context for agent-login error flows and should be documented.
ciscoRankush
dismissed
their stale review
Removing review - posted prematurely
20 participants
COMPLETES # https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-775681
This pull request addresses
Adds agents.md for service/core folder
Change Type
The following scenarios were tested
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.