feat(platform-server): add teams grpc client#1381
feat(platform-server): add teams grpc client#1381casey-brooks wants to merge 44 commits intomainfrom
Conversation
Summary
Testing
Test results: 153 passed, 0 failed, 0 skipped. |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Good structural approach — proto definitions are clean, module wiring follows existing patterns, config + env coverage is thorough, and the POST /api/graph removal is clean.
Two major items need addressing before merge:
-
Duplicate code in
TeamsGrpcClient— 28 public methods are near-identical 7-line copy-paste blocks (~450 lines). Extract a generic typed helper to make each method a one-liner. This is the biggest maintainability concern. -
No unit tests for
TeamsGrpcClient— Error mapping, constructor validation, timeout handling, and the custom exception class are all untested. These are pure-logic functions that are straightforward to cover.
Two minor items:
-
CANCELLEDgRPC status not explicitly handled — falls toBAD_GATEWAY, which is semantically wrong. -
Dead
GraphGuard— file and its test remain in-tree after removing the module provider. Clean up or add a TODO.
Summary
Testing
Test results: 810 passed, 0 failed, 12 skipped (822 tests) | Test files: 199 passed, 0 failed, 23 skipped (222). |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
All 4 prior review items verified in code:
-
Duplicate code →
callhelper ✓ —UnaryRpcCalltype + genericcall()method reduce 28 methods from 7-line bodies to clean 4-line delegations. Type casts are localized in the single helper. File reduced from 710 → 579 lines. -
Unit tests ✓ — 169 lines covering constructor validation, exhaustive gRPC→HTTP status mapping (via
it.each),extractServiceErrorMessageedge cases, default timeout, and per-call timeout overrides. Good coverage. -
CANCELLEDhandling ✓ — Explicitcase status.CANCELLEDin both switch statements (→ 499 /teams_cancelled). -
Dead
GraphGuard✓ —graph.guard.tsandapi.guard.mcp.command.test.tsboth deleted. No remaining references in the codebase.
Clean work. LGTM.
Summary
Verification
Testing
Test results: 810 passed, 0 failed, 12 skipped (822 tests) | Test files: 199 passed, 0 failed, 23 skipped (222). |
Summary
Testing
Test results: 810 passed, 0 failed, 12 skipped (822 total). Test files: 477 passed, 0 failed, 0 skipped (477 total). |
casey-brooks
force-pushed
the
noa/issue-1379
branch
from
eaa2309 to
f765553
Compare
Summary
Tests
|
Summary
Tests
|
Summary
Tests
|
Summary
Tests
|
|
All CI checks are now passing ✅ (Lint, Test Server, Test UI, Storybook, LiteLLM Integration, Build Server, Build UI). Changes since last review:
Requesting re-review. |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
All 4 items from the prior review are verified resolved in code. The additional @grpc/grpc-js → @connectrpc/connect migration across both docker-runner (server) and platform-server (client) is clean and well-tested:
- Unary endpoints correctly migrated from callback-based to async return
- Server streaming (logs, events) correctly uses async generators with queue buffering
- Bidi streaming (exec) properly adapted with request consumer loop, response queue, and
context.signalabort handling - New
armCompletionCheckpolling addresses the bidi stream completion detection gap Http2SessionManagerlifecycle properly managed in all integration testsbuf.gen.yamlcorrectly dropsconnect-esplugin (v2.x uses descriptors directly)@grpc/grpc-jsdependency fully removed from both packages
Two minor notes left as inline comments (not blocking):
OutOfRangemapped to HTTP status but missing a dedicated error code ingrpcStatusToErrorCodenormalizeBaseUrlduplicated between the two clients
LGTM.
Summary
Testing
Lint
|
Summary
Testing
Lint
|
Summary
Testing
Lint
|
|
Requesting re-review after UI migration changes. New commits since last approval:
What changed in the UI:
CI: All 7 checks green (Lint, Test Server, Test UI, Storybook, Build UI, Build Server, LiteLLM Integration). |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Re-review: UI Migration Commits
The server-side proto type renames (commit 97bceb8) are clean — straightforward CreateXRequest/CreateXResponse → XCreateRequest/X alignment.
The graph page removal, storybook cleanup, and route changes are also clean.
However, the new UI API client and entity mapper code (teamApi.ts, teamEntities.ts, useTeamEntities.ts, team.ts types) has systemic boundary/internal confusion that needs to be addressed before merge:
Core Issue: "Try Everything" Compatibility Pattern
The code consistently guesses at the API contract rather than implementing against a known one:
- Sends both
camelCaseANDsnake_casekeys in the same request body - Probes multiple field name variants when reading responses
- Sends all possible pagination parameter names (
pageSize,page_size,per_page) - Casts typed objects to
Record<string, unknown>to access undeclared fields
This is a hallmark of AI-generated code that hedges against an unknown API — but the gateway is your service with a known contract.
Structural Issues
- Types are maximally loose — all entity fields optional, enum unions widened with
| string | number - Normalization logic belongs at the boundary, not in feature code — 200+ lines of enum/field normalization are embedded in
teamEntities.tsinstead of inteamApi.ts - No unit tests for the 1105-line mapper module
- Duplicate utilities —
isRecord/isPlainRecordduplicated within the same file, andisRecord/readString/readNumberduplicated across files
Prior Unresolved Items (from last review)
The two minor items from the previous review remain unresolved:
OutOfRangestill missing fromgrpcStatusToErrorCodenormalizeBaseUrlstill duplicated between runner and teams clients
These are non-blocking but should be tracked.
packages/platform-ui/src/features/entities/hooks/useTeamEntities.ts
Outdated
Show resolved
Hide resolved
|
Summary:
Tests:
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
All 11 prior review comments verified as resolved in the actual code. Clean work.
What changed:
- Boundary layer (
teamApi.ts): Single naming convention, throws on unexpected shapes, typed request interfaces, enum validation viaSet-based guards. This is exactly how a boundary should work. - Types (
team.ts):id/createdAt/updatedAtrequired on response entities. Strict enum unions with no| string | numberwidening. Separate request types. - Feature code (
teamEntities.ts): Direct camelCase property access—no multi-key probing. Enum normalizers removed (handled at boundary). File dropped from 1105 to 860 lines. - Hooks (
useTeamEntities.ts):createAttachmentsends only{ kind, sourceId, targetId }. No moreRecord<string, unknown>casts or snake_case probing. - Shared utilities:
isRecord/readString/readNumberextracted tosrc/utils/typeGuards.ts. - Tests: New
teamEntities.test.tscovering mappers, request builders, attachment diffing, and config sanitization.
The two minor items from the earlier server-side review (OutOfRange in grpcStatusToErrorCode, normalizeBaseUrl duplication) are still unresolved but non-blocking for this PR. Recommend tracking separately.
|
Summary:
Tests:
|
|
Summary:
Tests:
|
|
Summary:
Tests:
|
Summary
Testing
|
Ready for review — Teams Graph Runtime IntegrationThis commit adds the bridge between the Teams gRPC service and the graph runtime, fixing three runtime bugs introduced by the Teams migration:
New files
Modified files
CI: All 7 checks passing ✅ |
Summary
Testing
Test results (from full run):
Lint: no errors |
Summary
Tests
|
Summary
Tests
|
Summary
Test & Lint
Tests: 784 passed, 0 failed, 6 skipped. |
Summary
Testing
|
Ready for ReviewAll 7 CI checks are passing. Summary of changes in this PR: Server (
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Re-review: Full FS Graph Removal (commits e7941b1..3691491)
All prior review feedback (across 5 review rounds) is verified resolved in the code. The new commits since the last LGTM cleanly complete the refactor:
Issue Coverage (all 11 phases verified)
- Proto + Client: Teams gRPC client with
callhelper, exhaustive error mapping, CANCELLED handling, unit tests ✓ - GraphRepository replacement:
TeamsGraphRepository→TeamsGraphSource.load(), abstract class simplified to singleload()✓ - Consumer updates:
liveGraph.manager.tsandmemory.service.tsuseload()(noget('main')) ✓ - FS infrastructure deleted:
fsGraph.repository.ts,hybridGraph.repository.ts,graphSchema.validator.ts,yaml.util.ts— all gone, zero stale references ✓ - NodeStateService removed: deleted file, removed from module/controllers, MCP tool discovery is in-memory only ✓
- GraphPersistController removed ✓
- Variables migrated to Teams RPCs: new
VariablesControllerusesTEAMS_GRPC_CLIENTdirectly ✓ - SlackTrigger removed: directory, module registration, template — all deleted ✓
- Cleanup: config fields, types, fixtures, tools/graph-ref-migrate — all removed ✓
- UI: clean boundary layer (
teamApi.ts), strict types, typed request interfaces, sharedtypeGuards.ts,teamEntities.test.ts✓ - Tests: FS graph tests deleted, stubs updated to
load(),TeamsGraphSource/HybridGraphRepository/TeamsGrpcClientall tested ✓
Architecture
The two-zone architecture is properly maintained:
- Boundary:
teamApi.ts(UI),VariablesController,TeamsGraphSource— all validate/parse/normalize external data - Internal:
LiveGraphRuntime,memory.service.ts, feature hooks — operate on canonical types only
Minor Items (non-blocking)
proto/grpc.tshas ~250 lines of dead Teams definitions added by this PR (inline comment)variables.controller.ts:80has unnecessary?? []on proto repeated field (inline comment)OutOfRangeingrpcStatusToErrorCodeandnormalizeBaseUrlduplication — tracked from prior reviews, non-blocking
Clean refactor. LGTM.
| TouchWorkloadRequestSchema, | ||
| TouchWorkloadResponseSchema, | ||
| } from './gen/agynio/api/runner/v1/runner_pb.js'; | ||
| import { |
There was a problem hiding this comment.
[minor] This PR adds ~250 lines of Teams service gRPC definitions (teamsServiceGrpcDefinition, TeamsServiceGrpcClient, all TEAMS_SERVICE_*_PATH constants) to this file, but none of them are imported anywhere in the codebase.
The TeamsGrpcClient (in teamsGrpc.client.ts) uses @connectrpc/connect with createClient(TeamsService, ...) and derives paths via teamsServicePath() from the TeamsService descriptor — it never touches this file.
Additionally, the runner definitions in this file appear to be dead as well (since the runner client also migrated to @connectrpc/connect), but those are pre-existing.
Consider removing the Teams additions from this file. If the entire file is now unused, it could be deleted entirely as a cleanup.
| }); | ||
| const response = await this.teamsClient.listVariables(request); | ||
| return { | ||
| items: response.variables ?? [], |
There was a problem hiding this comment.
[nit] response.variables is a proto repeated field — it always produces an array (defaults to [] in protobuf). The ?? [] fallback is unnecessary. Same for ?? undefined on nextPageToken (line 81) — proto string fields default to "", and the pagination utility's readString already handles empty strings.
items: response.variables,
nextPageToken: response.nextPageToken,
|
Summary:
Tests:
Lint: no errors. |
|
Test rerun:
Lint: not run (not requested). |
|
Storybook cleanup:
Tests/Lint:
|
Ready for Re-reviewAll 7 CI checks passing. New changes since last approval: Graph screen dead code removal (UI)
GraphRepository abstraction eliminated (Server)
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Re-review: 165c8d4 (prune builder UI and repos) + fe2998c (remove graph stories)
Clean continuation of the refactor. No new issues.
What changed
GraphRepository abstraction eliminated — The abstract class (graph.repository.ts) and its sole implementation (teamsGraph.repository.ts) are deleted. Consumers (LiveGraphRuntime, MemoryService) now inject TeamsGraphSource directly using the typed TeamsGraphSnapshot return type. The unnecessary PersistedGraph envelope (with name, version, updatedAt) is gone — correct, since those fields were synthetic fillers in the Teams path.
Graph builder UI removed (~4900 lines) — GraphLayout, GraphCanvas, EmptySelectionSidebar, NodeActionButtons, NodeDetailsPanel, NodeStatusBadges, NodeTracingSidebar, useGraphData, useGraphSocket, sidebarNodeItems, plus all associated tests and stories. Zero orphaned references remain. Surviving hooks (useNodeAction, useNodeStatus, useNodeTitleMap) and their tests correctly stay since they're still imported by live entity components.
8 test files updated — All GraphRepository stubs replaced with new TeamsGraphSource(createTeamsClientStub()) + vi.spyOn(...).mockResolvedValue(). Consistent pattern across all tests. The old StubRepo extends GraphRepository classes (with fake name, version, updatedAt fields) are gone.
DI wiring — graph-domain.module.ts exports TeamsGraphSource directly instead of through the GraphRepository → TeamsGraphRepository → TeamsGraphSource chain. Clean.
Prior unresolved threads (non-blocking)
Two [minor]/[nit] items from the previous review remain unaddressed:
- Dead Teams gRPC definitions in
proto/grpc.ts - Unnecessary
?? []on proto repeated field invariables.controller.ts:80
Neither blocks merge.
LGTM.
|
Removed the graph_state_tables migration directory. Tests/Lint:
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Re-review: 2d5f692 (remove graph state migration)
Correct removal of a bogus migration (20251222120000_graph_state_tables) that would have created GraphVariable and GraphNodeState tables with no corresponding Prisma models.
Verified:
- No
GraphVariable/GraphNodeStatemodels inschema.prisma - Zero references to either table in source code
- No downstream migration depends on these tables
- Migration timestamp places it after all other applied migrations — confirms it was introduced in this branch and never deployed
Clean. LGTM.
|
Merged main into noa/issue-1379 and resolved conflicts (kept proto:generate update, grpc-js bump, restored Teams env, added node_state emission). Tests/Lint:
|
|
Updated pnpm-lock.yaml via pnpm install to reflect grpc-js bump. Tests/Lint:
|
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Re-review: merge from main (c73aa71) + lockfile sync (cd71d43)
Verified the merge brought in two already-reviewed PRs from main:
#1372— notifications gRPC publisher (newNotificationsModule,NotificationsPublisherinjection intoGraphSocketGateway,emitNodeState,NodeStateEventSchema)#1399— docker-runner removal
No conflicts with PR #1381 code. The gateway changes are orthogonal — this PR removed graph-persist/snapshot events; the notifications PR adds a parallel gRPC notification channel. Config, module wiring, and test stubs all merge cleanly.
The lockfile commit is a single-line pnpm-lock.yaml change.
No functional changes to PR-specific code. LGTM.
3 participants
Summary
Testing
Closes #1379