Skip to content

docs(service): add root service docs#4747

Open
Shreyas281299 wants to merge 1 commit intowebex:task-refactorfrom
Shreyas281299:SDD-service-root
Open

docs(service): add root service docs#4747
Shreyas281299 wants to merge 1 commit intowebex:task-refactorfrom
Shreyas281299:SDD-service-root

Conversation

@Shreyas281299
Copy link
Contributor

COMPLETES #< INSERT LINK TO ISSUE >

This pull request addresses

< DESCRIBE THE CONTEXT OF THE ISSUE >

by making the following changes

< DESCRIBE YOUR CHANGES >

Change Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Tooling change
  • Internal code refactor

The following scenarios were tested

< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >

The GAI Coding Policy And Copyright Annotation Best Practices

  • GAI was not used (or, no additional notation is required)
  • Code was generated entirely by GAI
  • GAI was used to create a draft that was subsequently customized or modified
  • Coder created a draft manually that was non-substantively modified by GAI (e.g., refactoring was performed by GAI on manually written code)
  • Tool used for AI assistance (GitHub Copilot / Other - specify)
    • Github Copilot
    • Other - Please Specify
  • This PR is related to
    • Feature
    • Defect fix
    • Tech Debt
    • Automation

I certified that

  • I have read and followed contributing guidelines
  • I discussed changes with code owners prior to submitting this pull request
  • I have not skipped any automated checks
  • All existing and new tests passed
  • I have updated the documentation accordingly

Make sure to have followed the contributing guidelines before submitting.

@Shreyas281299 Shreyas281299 requested a review from a team as a code owner March 2, 2026 04:35
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b30e86b543

ℹ️ 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".

Comment on lines +139 to +143
| **Agent** | login, logout, state change, buddy agents, station, RONA | [`agent/ai-docs/AGENTS.md`](../agent/ai-docs/AGENTS.md) | [`agent/ai-docs/ARCHITECTURE.md`](../agent/ai-docs/ARCHITECTURE.md) |
| **Task** | task, hold, transfer, conference, wrapup, outdial, consult, accept, decline, state machine, XState, task states, guards, actions | [`task/ai-docs/AGENTS.md`](../task/ai-docs/AGENTS.md) | [`task/ai-docs/ARCHITECTURE.md`](../task/ai-docs/ARCHITECTURE.md) |
| **Config** | profile, register, teams, aux codes, desktop profile, org settings, dial plan | [`config/ai-docs/AGENTS.md`](../config/ai-docs/AGENTS.md) | [`config/ai-docs/ARCHITECTURE.md`](../config/ai-docs/ARCHITECTURE.md) |
| **Core** | websocket, HTTP, connection, reconnect, aqm, utils, errors, keepalive | [`core/ai-docs/AGENTS.md`](../core/ai-docs/AGENTS.md) | [`core/ai-docs/ARCHITECTURE.md`](../core/ai-docs/ARCHITECTURE.md) |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Replace service-doc routes that currently resolve to nothing

This routing table sends readers to agent/task/config/core ai-docs files that are not present under src/services/, while this document also says those docs should always be loaded before making changes. In practice that creates dead-end navigation for the documented workflow and makes the new “authoritative” guide non-actionable unless those targets are added or the links are changed to existing sources.

Useful? React with 👍 / 👎.

ContactCenter (cc.ts) — public API surface
├── WebexRequest.getInstance({webex}) ← initialized FIRST (singleton)
├── Services.getInstance({webex, config}) ← initialized SECOND (singleton)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use the actual Services.getInstance option key

The example call uses Services.getInstance({webex, config}), but Services is wired around connectionConfig (as seen in src/services/index.ts and the real call site in src/cc.ts). Copying this snippet passes no subscription config into ConnectionService, so the guide currently demonstrates an invocation shape that does not match runtime expectations.

Useful? React with 👍 / 👎.

Comment on lines +185 to +187
4. **Data services** (`AddressBook`, `EntryPoint`, `Queue`) — Created after `register()` succeeds and agent profile is available.
5. **`TaskManager`** — Created after `stationLogin()` succeeds.
6. **`WebCallingService`** — Created only when `loginOption === 'BROWSER'`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Correct the documented bootstrap sequencing

The “critical” order here says data services are created after register(), TaskManager after stationLogin(), and WebCallingService only for BROWSER login, but ContactCenter instantiates all three in the READY handler before either API call (src/cc.ts constructor). This contradiction makes the architecture guidance unreliable for troubleshooting initialization issues or extending startup logic.

Useful? React with 👍 / 👎.

@Shreyas281299 Shreyas281299 added the validated If the pull request is validated for automation. label Mar 2, 2026
Copy link
Contributor Author

@Shreyas281299 Shreyas281299 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: docs(service): add root service docs

Overall

The documentation is well-structured and comprehensive -- the file structure tree, key capabilities table, architecture diagrams, bootstrap order, and request/response flow patterns are all clear and useful. This will be a strong reference for AI agents and developers working in the services layer.

Issues to Address

1. PR Template Not Filled Out (Compliance)

The PR description still has the default template placeholders, and none of the checkboxes are ticked. The template itself states:

FAILING TO FILL OUT THIS TEMPLATE WILL RESULT IN REJECTION OF YOUR PULL REQUEST. This is for compliance purposes with FedRAMP program.

Please fill out the PR description -- at minimum the "addresses", "changes", "Change Type" (Documentation update), GAI policy, and "I certified" sections.

2. Verify Relative Links to Package Root

Several links in the "Related" section point three levels up to the package root. Please verify these paths exist on the task-refactor branch:

  • ../../../AGENTS.md resolves to packages/@webex/contact-center/AGENTS.md
  • ../../../ai-docs/RULES.md resolves to packages/@webex/contact-center/ai-docs/RULES.md
  • ../../../ai-docs/patterns/ resolves to packages/@webex/contact-center/ai-docs/patterns/
  • ../../../ai-docs/patterns/typescript-patterns.md

If these do not exist yet (or are planned for a future PR), consider noting them as "TODO" or removing the dead links.

3. Sub-Service ai-docs Existence

The Service Routing table links to ai-docs under agent/, task/, config/, core/, and task/state-machine/. Are these already merged or in-flight? If they do not exist yet, readers/agents following these links will hit dead ends. Consider adding a note like: "These docs are being added incrementally -- check the folder for availability."

Suggestions (Non-blocking)

4. Consider Adding an ARCHITECTURE.md Companion

Other service folders (agent, task, config, core) have both AGENTS.md and ARCHITECTURE.md. For consistency, consider whether a companion ARCHITECTURE.md would be useful here -- even if it is lighter-weight, covering just the Services singleton internals and construction details.

5. PageCache Note Placement

The note about src/utils/PageCache.ts at the bottom of the File Structure section feels slightly out of place. Consider moving it to the Data Services Pattern section where PageCache is directly referenced and more contextually relevant.

Summary

Good documentation overall. Main action items: (1) fill out the PR template for compliance, (2) verify the relative links, (3) clarify sub-service docs availability.

Copy link

@ciscoRankush ciscoRankush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Service Root-level AGENTS.md

Good overall structure — the service routing table, architecture diagram, and AqmReqs factory pattern documentation are valuable for AI agent navigation. A few accuracy issues need fixing before merge, mostly around bootstrap order and event flow.

3 Important | 2 Medium | 2 Minor

ContactCenter (cc.ts) — public API surface
├── WebexRequest.getInstance({webex}) ← initialized FIRST (singleton)
├── Services.getInstance({webex, config}) ← initialized SECOND (singleton)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Important] Services.getInstance({webex, config}) — the second parameter is connectionConfig, not config.

Actual signature in services/index.ts:39:

constructor(options: {webex: WebexSDK; connectionConfig: SubscribeRequest})

Suggest:

├── Services.getInstance({webex, connectionConfig}) ← initialized SECOND (singleton)

Comment on lines +170 to +187
├── TaskManager ← task lifecycle, created after login
├── WebCallingService ← WebRTC calling, created on BROWSER login
├── AddressBook ← REST data service, created after register
├── EntryPoint ← REST data service, created after register
├── Queue ← REST data service, created after register
└── MetricsManager.getInstance({webex}) ← telemetry singleton
```

### Bootstrap Order (Critical)

Understanding the instantiation order is essential — getting it wrong causes runtime errors:

1. **`WebexRequest.getInstance({webex})`** — Must be called first. The singleton HTTP client that all services depend on.
2. **`Services.getInstance({webex, connectionConfig})`** — Creates `WebSocketManager`, `AqmReqs`, `ConnectionService`, `AgentConfigService`, `routingAgent`, `routingContact`, `aqmDialer`.
3. **`MetricsManager.getInstance({webex})`** — Telemetry singleton.
4. **Data services** (`AddressBook`, `EntryPoint`, `Queue`) — Created after `register()` succeeds and agent profile is available.
5. **`TaskManager`** — Created after `stationLogin()` succeeds.
6. **`WebCallingService`** — Created only when `loginOption === 'BROWSER'`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Important] Bootstrap order is inaccurate. In cc.ts:register() (lines 350-376), all of these are created during register():

  1. WebexRequest
  2. Services
  3. WebCallingService (always instantiated; only registerWebCallingLine() is conditional on BROWSER)
  4. MetricsManager
  5. TaskManager
  6. Data services (AddressBook, EntryPoint, Queue)
  • Line 170: TaskManager is not created "after login" — it's created in register().
  • Line 171: WebCallingService is not "created on BROWSER login" — it's always created; only line registration is conditional.
  • Lines 185-187: Same errors in the numbered list.

Same errors repeat in "What Services does NOT create" section (lines 250-251): "TaskManager — created after successful station login" and "WebCallingService — created only for BROWSER login" are both inaccurate.

Comment on lines +354 to +362
→ WebSocketManager emits 'message'
→ AqmReqs.onMessage() — checks if message correlates to a pending request
→ If match: resolves/rejects the pending Promise
→ If no match: ignored by AqmReqs
→ cc.ts webSocketManager.on('message') — routes to appropriate handler:
→ Agent events → emitted on cc EventEmitter (AGENT_EVENTS)
→ Task events → forwarded to TaskManager → Task state machine
→ Connection events → handled by ConnectionService
```

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Important] This event flow implies cc.ts is a central router that forwards events to TaskManager and ConnectionService. In reality, there are 4 independent listeners on webSocketManager.on('message'):

  1. AqmReqsaqm-reqs.ts:20 (registered in AqmReqs constructor)
  2. cc.tscc.ts:358 (registered in register())
  3. TaskManagerTaskManager.ts:314 (registered in registerTaskListeners())
  4. ConnectionServiceconnection-service.ts:44-45 (registered in constructor)

cc.ts does not route or forward events to TaskManager or ConnectionService — each listener independently processes the same WebSocket messages.


[Medium] Also, line 359 says "Agent events → emitted on cc EventEmitter" but cc.ts uses two different emission mechanisms: this.emit() (EventEmitter) for agent events and this.trigger() (WebexPlugin) for task events (cc.ts:1075-1177). This distinction matters for consumers subscribing to events.

```
src/services/
├── index.ts # Services singleton — composes all services
├── constants.ts # Shared constants (WCC_API_GATEWAY, API paths)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] constants.ts only gets a one-liner here. This file contains important shared constants that AI agents need when adding new endpoints or services:

  • WCC_API_GATEWAY — service identifier used in all HTTP requests
  • SUBSCRIBE_API, LOGIN_API, STATE_CHANGE_API — API path constants
  • WEB_RTC_PREFIX — prefix for WebRTC endpoints
  • WEBSOCKET_EVENT_TIMEOUT — 20000ms timeout
  • DEFAULT_RTMS_DOMAIN, WCC_CALLING_RTMS_DOMAIN — WebRTC domain constants
  • METHODS object — WebCallingService method name constants

Consider adding a brief "Shared Constants" section or expanding this annotation so AI agents discover these naming conventions.


> **Note**: The task state machine (`task/state-machine/`) is part of the Task service, not a separate service. Its dedicated docs live at [`task/state-machine/ai-docs/AGENTS.md`](../task/state-machine/ai-docs/AGENTS.md) and [`ARCHITECTURE.md`](../task/state-machine/ai-docs/ARCHITECTURE.md). Load these when working on state transitions, guards, or actions.

**Data services** (AddressBook, Queue, EntryPoint) and **WebCallingService** do not have dedicated ai-docs. For these, read the source files directly — they follow the same patterns documented in [`ai-docs/patterns/typescript-patterns.md`](../../../ai-docs/patterns/typescript-patterns.md).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] WebCallingService does not follow "the same patterns documented in typescript-patterns.md" as data services. Data services use REST + pagination + PageCache caching. WebCallingService extends EventEmitter, wraps @webex/calling (createClient/ICallingClient/ILine/ICall), manages call lifecycle with a callTaskMap, and has an async registration flow with timeout — a completely different pattern. Consider noting this distinction rather than grouping it with data services.

│ │ ├── constants.ts # TaskState, TaskEvent enums
│ │ ├── types.ts # TaskContext type
│ │ ├── guards.ts # State transition guards
│ │ ├── actions.ts # State transition actions

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] File tree omits state-machine/index.ts — this file exists and serves as the barrel export for all state machine public API (createTaskStateMachine, TaskState, TaskEvent, guards, actions, createInitialContext, types). Should be listed here alongside the other state machine files.

| **Caching** | `PageCache<T>` — caches pages for simple pagination, bypasses cache for search/filter |
| **Metrics** | `timeEvent` on API call start, `trackEvent` on success/failure |
| **Logging** | `LoggerProxy` with `{module: 'ClassName', method: 'methodName'}` context |
| **Error handling** | try/catch with `LoggerProxy.error` + `metricsManager.trackEvent` for failures |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Minor] Error handling pattern says "try/catch with LoggerProxy.error + metricsManager.trackEvent" but omits that data services re-throw the error after logging (e.g., AddressBook.ts catch block logs then throws). The re-throw is important — it means errors propagate to callers rather than being silently swallowed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

validated If the pull request is validated for automation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants