docs(contact-center): add core service AI documentation-Architecture#4729
docs(contact-center): add core service AI documentation-Architecture#4729Shreyas281299 wants to merge 2 commits intowebex:nextfrom
Conversation
Shreyas281299
added
the
validated
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
| --- | ||
|
|
||
| ## Related Files | ||
|
|
There was a problem hiding this comment.
need navigation back to the orchestrator. Add link to ../../../AGENTS.md
|
|
||
| ```mermaid | ||
| sequenceDiagram | ||
| participant App |
There was a problem hiding this comment.
Instead of App here we should the component name from where initWebSocket gets called. I believe it's cc object
| F -->|No| H[Reject with error] | ||
| ``` | ||
|
|
||
| ### Configuration Structure |
There was a problem hiding this comment.
This data seems redundant from what we have captured in AGENTS.md file also
|
|
||
| ### Class Structure | ||
|
|
||
| ```typescript |
There was a problem hiding this comment.
Here also we are capturing same information as AGENTS.md. We should be adding information here like Properties under the class, methods under the class. Event detail, it's payload structure reference. Also why it needs to extend eventemitter
|
|
||
| ### getErrorDetails Flow | ||
|
|
||
| ```mermaid |
There was a problem hiding this comment.
This diagram has some parsing error so display fails
K --> L[Return {error, reason}] - This line has the error
|
|
||
| --- | ||
|
|
||
| ## Component Overview |
There was a problem hiding this comment.
We already have Key Component/Key capabilities sections added in AGENTS.md files so we can just directly get into detailed architectural design for each of those components here. And if an overview section is required then adding a high level diagram of how all these components are working together and then detailed design for each of those. Also talking about entrypoint for this core folder from outside core would also be something we should add
|
|
||
| --- | ||
|
|
||
| ## File Structure |
There was a problem hiding this comment.
We should keep these in Agents.md file maybe
| participant App | ||
| participant WSM as WebSocketManager | ||
| participant WS as WebSocket | ||
| participant BE as Backend |
There was a problem hiding this comment.
BAckend should say CC Backend
|
|
||
| --- | ||
|
|
||
| ## Utility Functions |
There was a problem hiding this comment.
Keep the complete error section together
- Replace component overview table with high-level architecture diagram - Remove file structure section (belongs in AGENTS.md) - Enhance WebSocketManager with properties, methods, events, and EventEmitter rationale - Fix sequence diagram: App -> CC object, Backend -> CC Backend - Fix mermaid parsing error in getErrorDetails flowchart - Remove redundant AqmReqs config, reference AGENTS.md instead - Consolidate error handling sections together - Add navigation links to orchestrator and core AGENTS.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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: acdf6ac9e8
ℹ️ 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".
|
|
||
| ## High-Level Architecture | ||
|
|
||
| The core service is the foundational infrastructure layer used by all other services in the contact center SDK. The entry point into core from outside is through `Services` (`src/services/index.ts`), which instantiates `WebSocketManager` and `WebexRequest` during initialization. The `cc.ts` plugin class calls `Services.init()` on registration, which bootstraps the core components. |
There was a problem hiding this comment.
The overview currently documents a startup path that does not exist: Services.init() is never called, and Services does not initialize WebexRequest. In the actual code, cc.ts first calls WebexRequest.getInstance({webex}) and then Services.getInstance(...), while services/index.ts only creates WebSocketManager/AqmReqs. This mismatch is actionable because maintainers following this doc can wire startup in the wrong order and hit WebexRequest.getInstance() before initialization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
PR #4729 Review: docs(contact-center): add core service AI documentation-Architecture
Blocking (Must Fix)
B1. Services.init() does not exist (line 6, diagram line 10)
Doc says cc.ts calls Services.init() but the actual call is Services.getInstance(...) (cc.ts:354). Also, the diagram claims Services creates WebexRequest singleton, but WebexRequest.getInstance() is initialized by cc.ts:350-352 BEFORE Services.getInstance(). AqmReqs internally calls WebexRequest.getInstance() (aqm-reqs.ts:18) relying on it being already created. The bootstrap ordering is fundamentally wrong.
B2. Failure type structure is wrong (lines 264-276)
Actual type (GlobalTypes.ts:23-34) is Msg<{agentId: string; trackingId: string; reasonCode: number; orgId: string; reason: string}> where Msg<T> provides top-level type, orgId, trackingId, and data: T. Doc makes orgId/trackingId optional (they're required), makes data optional (it's required), and shows reasonCode as string | number (it's number only).
B3. AugmentedError type shown with specific fields (lines 278-289)
Actual type (GlobalTypes.ts:59-61) is Error & {data?: Record<string, any>} -- a generic flexible type, NOT the specific shape shown. While the specific fields reflect runtime usage in generateTaskErrorObject, the type contract is Record<string, any>.
B4. WebexRequest.uploadLogs signature is wrong (lines 170-171)
Doc: uploadLogs(options?: {correlationId?: string}): Promise<UploadResponse>
Actual (WebexRequest.ts:53): uploadLogs(metaData: LogsMetaData = {}): Promise<UploadLogsResponse> where LogsMetaData also has feedbackId?. Wrong param name, wrong type, wrong return type.
B5. WebexRequest.request signature is wrong (lines 168-169)
Doc: request(config: RequestConfig): Promise<Response>
Actual (WebexRequest.ts:32-37): request(options: {service: string; resource: string; method: HTTP_METHODS; body?: RequestBody}): Promise<IHttpResponse>. RequestConfig and Response types don't exist.
B6. webSocketOnCloseHandler return type wrong (line 81)
Actual method is private async webSocketOnCloseHandler(event: any) returning Promise<void>, not void as documented.
B7. Mermaid diagram parsing error (line ~209)
K --> L["Return {error, reason}"] -- curly braces inside Mermaid node labels cause parsing errors. Already flagged by Kesari3008.
Suggestions (Should Fix)
S1. ConnectionService section missing methods, reconnection flow, and constants (lines 137-157) -- Missing: setConnectionProp, onPing, onSocketClose, handleSocketClose, handleConnectionLost, handleRestoreFailed, dispatchConnectionEvent. Also missing key constants: LOST_CONNECTION_RECOVERY_TIMEOUT (50s), WS_DISCONNECT_ALLOWED (8s), CONNECTIVITY_CHECK_INTERVAL (5s).
S2. keepalive.worker.js docs omit offline detection and forced closure logic (lines 322-344) -- Actual worker has checkNetworkStatus(), resetOfflineHandler(), tracks initialised/initiateWebSocketClosure flags, and listens for online/offline events. The simplified version misses the core purpose of the worker.
S3. AqmReqs section missing bind-matching internals (lines 126-134) -- Missing: pendingRequests/pendingNotifCancelrequest maps, bindPrint/bindCheck pattern, TIMEOUT_REQ = 20000ms, notifCancel mechanism.
S4. generateTaskErrorObject code omits errorData in Error message (lines 295-316) -- Actual: const reason = \${errorType}: ${errorMessage}${errorData ? ` (${errorData})` : ''}`;Doc omits theerrorData` appendage.
S5. Diagram missing Services --> creates --> ConnectionService relationship (lines 8-27) -- Services constructor creates ConnectionService (services/index.ts:47-50) but diagram doesn't show this.
S6. Diagram missing several components created by Services -- Also creates AgentConfigService, routingAgent, routingContact, aqmDialer.
Nits
- N1.
WebexRequest.getInstancecode usesoptions && options.webexnotoptions?.webex-- doc should match actual code - N2.
Err.Message/Err.Detailsclasses referenced in diagram as "Error class factories" but never explained
| > **Purpose**: Technical documentation for core infrastructure components. | ||
|
|
||
| --- | ||
|
|
There was a problem hiding this comment.
[Blocking] Services.init() does not exist. The actual call is Services.getInstance(...) (cc.ts:354). Also, Services does NOT create WebexRequest -- cc.ts:350-352 initializes WebexRequest.getInstance({webex}) BEFORE calling Services.getInstance(). The Mermaid diagram line SVC -->|creates| WR[WebexRequest singleton] is wrong; it should show CC creating WR and SVC using it transitively via AqmReqs.
| ) => { | ||
| const failure = error.details as Failure; | ||
| const reason = failure?.data?.reason ?? `Error while performing ${methodName}`; | ||
|
|
There was a problem hiding this comment.
[Blocking] Failure type is structurally wrong. Actual type (GlobalTypes.ts:23-34) is Msg<{agentId: string; trackingId: string; reasonCode: number; orgId: string; reason: string}>. Key differences:
orgIdandtrackingIdare NOT optional (required inMsg<T>)datais NOT optional (required inMsg<T>)reasonCodeisnumberonly, notstring | number- All fields inside
dataare required, not optional
|
|
||
| ```typescript | ||
| export default class WebexRequest { | ||
| private static instance: WebexRequest; |
There was a problem hiding this comment.
[Blocking] request method signature is wrong.
Doc: request(config: RequestConfig): Promise<Response>
Actual (WebexRequest.ts:32-37): request(options: {service: string; resource: string; method: HTTP_METHODS; body?: RequestBody}): Promise<IHttpResponse>
Neither RequestConfig nor Response types exist in the codebase.
| export default class WebexRequest { | ||
| private static instance: WebexRequest; | ||
| private webex: WebexSDK; | ||
|
|
There was a problem hiding this comment.
[Blocking] uploadLogs signature is wrong.
Doc: uploadLogs(options?: {correlationId?: string}): Promise<UploadResponse>
Actual (WebexRequest.ts:53): uploadLogs(metaData: LogsMetaData = {}): Promise<UploadLogsResponse>
LogsMetaData has both feedbackId? and correlationId?. Wrong param name, wrong type name, missing field, wrong return type.
|
|
||
| const err = new Error(reason); | ||
| err.data = errData; | ||
| return {error: err, reason}; |
There was a problem hiding this comment.
[Blocking] AugmentedError type shown with specific fields but the actual type (GlobalTypes.ts:59-61) is:
export interface AugmentedError extends Error {
data?: Record<string, any>;
}The data field is Record<string, any>, not the specific shape shown. While the fields here reflect runtime usage in generateTaskErrorObject, the actual type contract is generic.
| // Handle disconnect | ||
| } else if (details.isSocketReconnected) { | ||
| // Handle reconnect | ||
| } |
There was a problem hiding this comment.
[Suggestion] ConnectionService section is too sparse. Missing:
- Constructor parameters (
ConnectionServiceOptions) - Key methods:
setConnectionProp,onPing,onSocketClose,handleSocketClose,handleConnectionLost,handleRestoreFailed,dispatchConnectionEvent - Reconnection flow with
setIntervalandCONNECTIVITY_CHECK_INTERVAL(5s) - Key constants:
LOST_CONNECTION_RECOVERY_TIMEOUT(50s),WS_DISCONNECT_ALLOWED(8s)
| let intervalId; | ||
|
|
||
| addEventListener('message', (event) => { | ||
| if (event.data?.type === 'start') { |
There was a problem hiding this comment.
[Suggestion] keepalive.worker.js docs are oversimplified. The actual worker has:
checkNetworkStatus()function with offline detectionresetOfflineHandler()for clearing offline-related timeoutsinitialisedandinitiateWebSocketClosurestate flagsonline/offlineevent listeners onself- Forced WebSocket closure via
postMessage({type: 'closeSocket'})after timeout
The offline detection and forced closure logic is a core purpose of the worker.
ciscoRankush
dismissed
their stale review
Removing review - posted prematurely
3 participants
COMPLETES # https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-775682
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.