feat: add Data Fabric roles and directory services

[dsuresh-ap]

Summary

• Add DataFabricRoles and DataFabricDirectory services to the SDK surface.
• Introduce typed models for role listing, directory listing, and role assignment payloads/results.
• Wire the new services into UiPath legacy accessors and export them from the Data Fabric module.
• Add unit coverage for role listing, directory paging, and role assignment behavior.

Testing

• Unit tests added for Data Fabric roles and directory services.
• Legacy UiPath service wiring test updated to verify the new accessors are exposed.
• Tested against alp

[claude]

Review summary

Four inline comments posted. Two additional checklist items from the post-implementation verification checklist in agent_docs/rules.md:

Missing: integration tests
Every new method requires an integration test in tests/integration/shared/data-fabric/. The PR description notes these were not run/written. Per conventions:

Every new method must also have an integration test in tests/integration/shared/{domain}/. These run against a live API and catch issues unit tests miss — wrong endpoints, broken transforms, auth/header problems.

Both DataFabricRoleService.getAll and all three DataFabricDirectoryService methods need integration test coverage.

Missing: docs/oauth-scopes.md update
Per conventions:

When adding methods, update docs/oauth-scopes.md with required OAuth scopes. NEVER skip this — missing scopes break the OAuth integration guide.

The new /api/v2/Role and /api/Directory endpoints will require specific OAuth scopes — these need to be documented.

[claude]

Review summary

Two new findings this run:

1. DirectoryPaginationOptions belongs in directory.internal-types.ts (line 31)
Service-internal operation type defined at module scope in the service file - conventions require it to be in *.internal-types.ts.

2. assignRoles calling this.getAll() fires spurious telemetry (line 255)
Every assignRoles call with preserveExisting: true fires both DataFabricDirectory.AssignRoles and DataFabricDirectory.GetAll, inflating GetAll metrics. Fix: extract a private un-tracked fetchAllEntries() helper and have both getAll() and assignRoles delegate to it.

[claude]

Review summary

Three new findings this pass (prior four threads are open and unchanged):

New inline comments posted:

1. src/utils/pagination/helpers.ts line 368 — getAllPages options parameter typed as Record<string, any>. Violates the no-any rule; should use a constrained generic like PaginationHelpers.getAll does.

2. src/services/data-fabric/directory.ts line 240 — assignRoles calls this.getAll(), which is decorated with @track('DataFabricDirectory.GetAll'). Every assignRoles(preserveExisting: true) call fires a spurious DataFabricDirectory.GetAll telemetry event. Fix: extract the internal fetch into a private untracked fetchAllEntries() helper.

3. tests/integration/shared/data-fabric/access.integration.test.ts line 91 — describe.skip used to skip a write test for mutation-safety reasons. Convention only permits describe.skip for PAT-auth failures; mutation risk should be handled with a beforeAll guard that throws unless a dedicated env var is set.

[claude]

Review summary

Three new findings this pass:

1. tests/unit/utils/pagination/helpers.test.ts line 862 — let mockConfig: any violates the no-any rule. Should use GetAllConfig<RawType, TransformedType> from the helpers module.

2. tests/unit/services/data-fabric/directory.test.ts line 29 — list and getAll describe blocks have no error scenario tests. Convention requires testing both success and error paths for every public method.

3. tests/unit/services/data-fabric/roles.test.ts line 27 — getAll describe block has no error scenario test. Same convention gap.

[claude]

Review summary

One new finding this run:

src/utils/constants/common.ts line 64 — DIRECTORY_PAGINATION.ITEMS_FIELD and TOTAL_COUNT_FIELD only handle lowercase results/totalCount, while extractDirectoryItems() and extractTotalCount() (used by list()) also handle Results/TotalCount. If the live API sends PascalCase fields, getAll() silently returns [] while list() returns the correct data. The presence of both casings in RawDataFabricDirectoryListResponse suggests the actual API response format was not verified before the PR was written.

[claude]

Review summary

One new finding this pass:

src/models/data-fabric/directory.types.ts line 41 — type?: string | number uses raw primitives when DataFabricDirectoryEntityType already exists for this fixed value set. Suggested DataFabricDirectoryEntityType | string to preserve enum semantics while staying defensive against string API responses.

[claude]

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

[claude]

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

[Sarath1018]

question: API allows only to revoke all roles? cant we revoke a particular role?

[dsuresh-ap]

@Sarath1018 The UI revokes roles using the POST /datafabric_/api/Directory/Role. You pass the roles you want the end state to be.

revokeRoles is to match the UI behavior /datafabric_/api/Directory/RevokeRole for removing all roles since the above endpoint doesn't allow empty array.

[claude]

This CI-aware file exclusion is functionally equivalent to describe.skipIf and has the same problem the convention prohibits:

Do not use describe.skip for missing config — those require a beforeAll guard or a throw.

When DATA_FABRIC_ACCESS_INTEGRATION is not set in CI, the file is silently excluded from the test run. The beforeAll guard in access.integration.test.ts never fires, so CI passes while the tests are not exercised at all. That is exactly the silent-skip behaviour the convention was written to prevent.

The beforeAll + throw in the test file is already the correct gate. Remove the config-level exclusion and rely solely on it:

Suggested change

	const ciOptionalIntegrationExcludes = process.env.CI === "true"
	&& process.env.DATA_FABRIC_ACCESS_INTEGRATION !== "true"
	? ["tests/integration/shared/data-fabric/access.integration.test.ts"]
	: [];

Then in vitest.integration.config.ts:

Suggested change

	const ciOptionalIntegrationExcludes = process.env.CI === "true"
	&& process.env.DATA_FABRIC_ACCESS_INTEGRATION !== "true"
	? ["tests/integration/shared/data-fabric/access.integration.test.ts"]
	: [];
	exclude: [
	"tests/integration/shared/maestro/**",
	],

When DATA_FABRIC_ACCESS_INTEGRATION is absent in CI, the beforeAll throw will make the suite fail visibly — which is the intended behaviour so someone notices the test is not being exercised.

[claude]

Review summary

One new finding this run:

vitest.integration.config.ts lines 3–6 — ciOptionalIntegrationExcludes silently drops access.integration.test.ts from CI runs when DATA_FABRIC_ACCESS_INTEGRATION is not set. This is functionally equivalent to the describe.skipIf pattern that was removed from the test files in prior rounds — it just moved the silent skip to the build config. The beforeAll guard that throws in access.integration.test.ts never fires when the file is excluded, so CI passes while the tests are not exercised. Per convention, the beforeAll + throw should be the only gate. Remove ciOptionalIntegrationExcludes from vitest.integration.config.ts and rely solely on the existing guard in the test file.

[Sarath1018]

nit: Can we drop the changes to the test-barrel index.ts files as they dont serve any purpose

[Sarath1018]

Lets not use this approach to not run integration tests.

Use describe.skip by commenting 403 reason by mentioning the scopes issue.

[claude]

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
94.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud