Route team config through project service - #314
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughIntroduces a project-service-backed team role configuration feature: new API contract types and routes, MetadataServer storage/HTTP handlers, daemon core-text routing, CLI rewrite from local config to project API calls, an installed shim ChangesTeam role configuration feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Shim as installed-aimux-shim.sh
participant Daemon
participant ProjectService as MetadataServer
User->>Shim: aimux team add role ...
Shim->>Daemon: GET /core/team/add-text
Daemon->>ProjectService: POST team.addRole { role, description, reviewedBy, canEdit }
ProjectService->>ProjectService: mutate roles/defaultRole, saveTeamConfig
ProjectService-->>Daemon: TeamConfigResponse
Daemon->>Daemon: teamPayloadFromResult + renderCoreTeamAddLines
Daemon-->>Shim: plain text response
Shim-->>User: print output
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/metadata-server.ts (1)
1282-1296: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueFallback default role is a hardcoded magic string that can end up dangling.
If the removed role is the last remaining role (and is also the current
defaultRole),config.defaultRolefalls back to"coder"even though"coder"no longer exists inconfig.roles. Deriving the fallback fromgetDefaultTeamConfig().defaultRole(or leavingdefaultRoleunset whenrolesis empty) would avoid both the magic-string duplication and the dangling reference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/metadata-server.ts` around lines 1282 - 1296, The fallback handling in removeTeamRole is using a hardcoded "coder" default that can point to a deleted role. Update the defaultRole reassignment in removeTeamRole to derive the fallback from getDefaultTeamConfig().defaultRole or otherwise leave it unset when config.roles becomes empty, and keep the logic aligned with loadTeamConfig/saveTeamConfig so defaultRole always references an existing role.src/core-text.ts (2)
86-92: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOptional
roleweakens type safety for action-specific renderers.
roleis optional here, butrenderCoreTeamAddLines/renderCoreTeamRemoveLines/renderCoreTeamDefaultLines(Lines 359-369) assume it's always set. Nothing in the type system enforces that callers ofteamPayloadFromResultpassrolefor those actions, so a future omission would silently renderRole "undefined" saved.instead of failing at compile time.Consider a discriminated payload (e.g. a variant requiring
role: stringfor add/remove/default) to make the contract explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-text.ts` around lines 86 - 92, The team payload contract in teamPayloadFromResult is too loose because role is optional even though renderCoreTeamAddLines, renderCoreTeamRemoveLines, and renderCoreTeamDefaultLines require it for action-specific rendering. Update the payload typing to use a discriminated union or equivalent variant that requires role for the add/remove/default cases, and keep teamPayloadFromResult aligned with those required fields so missing role becomes a compile-time error instead of rendering undefined.
359-369: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGuard against missing
rolebefore rendering.These functions interpolate
payload.role(optional) directly into user-facing output. If a caller forgets to supplyrole, the message silently becomesRole "undefined" saved.with no error.🛡️ Proposed defensive check
export function renderCoreTeamAddLines(payload: CoreTeamTextPayload): string[] { + if (!payload.role) return ["Error: role is required for this operation."]; return [`Role "${payload.role}" saved.`]; }Apply similarly to
renderCoreTeamRemoveLinesandrenderCoreTeamDefaultLines, or better, enforcerole: stringvia a discriminated payload type as noted above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-text.ts` around lines 359 - 369, The rendering helpers are using payload.role without guarding against it being missing, which can produce user-facing text like undefined; update renderCoreTeamAddLines and apply the same fix in renderCoreTeamRemoveLines and renderCoreTeamDefaultLines by either validating role before interpolation or tightening the payload type so role is required, and ensure the functions fail fast or handle the absence explicitly rather than rendering an undefined value.src/daemon.ts (1)
1583-1653: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winTeam mutation routes skip the standard mutation timeout.
teamInitTextRouteandteamRoleTextRoutecallpostProjectServiceJsonwithout{ timeoutMs: CLI_PROJECT_MUTATION_TIMEOUT_MS }, unlike other project-service mutation routes in this file (threadOpenTextRoute,handoffMutationTextRoute,taskMutationTextRoute,worktreePathTextRoute,graveyardAgentTextRoute,reviewRequestChangesTextRoute). This falls back to the shorter defaultPROXY_TIMEOUT_MS, which could cause spurious timeout errors on slower team.json writes.🔧 Proposed fix
private async teamInitTextRoute(routeUrl: URL, body: unknown): Promise<DaemonRouteResponse> { const project = this.requiredParam(routeUrl, body, "project"); if (typeof project !== "string") return project; - const result = await this.postProjectServiceJson(project, PROJECT_API_ROUTES.team.init, {}); + const result = await this.postProjectServiceJson(project, PROJECT_API_ROUTES.team.init, {}, { + timeoutMs: CLI_PROJECT_MUTATION_TIMEOUT_MS, + }); const payload = this.teamPayloadFromResult(result, "team init");const result = await this.postProjectServiceJson(project, input.routePath, { role, ...(input.extraBody ? input.extraBody(role) : {}), - }); + }, { timeoutMs: CLI_PROJECT_MUTATION_TIMEOUT_MS });Separately, verifying this new daemon routing end-to-end requires building and running against an installed daemon rather than source inspection alone.
[reliability_and_availability]
Based on path instructions: "For aimux runtime or CLI behavior, source-level validation is not enough: build, install the local release asset, and run
aimux restartso the daemon, services, and dashboards pick up the updated bundle."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/daemon.ts` around lines 1583 - 1653, The team mutation routes are missing the standard mutation timeout, so update `teamInitTextRoute` and `teamRoleTextRoute` to pass the same `{ timeoutMs: CLI_PROJECT_MUTATION_TIMEOUT_MS }` option used by other mutation helpers in this file. Apply the change on the `postProjectServiceJson` calls in those methods so `team.json` writes use the longer mutation timeout instead of the default proxy timeout.Source: Path instructions
src/installed-shim.test.ts (1)
745-774: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFallback test doesn't confirm the staleness check actually ran.
The assertions here only verify the Node launcher was invoked with expected args/exit codes; they don't check
curlLogfor a daemon-status/health probe. If the shim's staleness-detection logic regressed to unconditionally launching Node forteamcommands, this test would still pass.Consider adding a
curlLogassertion (similar to the matching-daemon test) to confirm a health check actually occurred before the Node fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/installed-shim.test.ts` around lines 745 - 774, The fallback test in the team daemon staleness case only checks Node launcher invocation, so it can pass even if the health probe never runs. Update this test to assert the health/status check occurred by inspecting curlLog, similar to the matching-daemon test, alongside the existing run() and nodeLog assertions in the makeFixture-based scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/metadata-server.ts`:
- Around line 1257-1280: The addTeamRole method in metadata-server should
preserve existing RoleConfig fields when updating an already-defined role
instead of overwriting them with only the provided inputs. Update the nextRole
निर्माण in addTeamRole so it falls back to config.roles[role]?.reviewedBy and
config.roles[role]?.canEdit when those inputs are omitted, similar to how
description already preserves the prior value, and keep the existing
saveTeamConfig and notifyProjectChanged flow unchanged.
---
Nitpick comments:
In `@src/core-text.ts`:
- Around line 86-92: The team payload contract in teamPayloadFromResult is too
loose because role is optional even though renderCoreTeamAddLines,
renderCoreTeamRemoveLines, and renderCoreTeamDefaultLines require it for
action-specific rendering. Update the payload typing to use a discriminated
union or equivalent variant that requires role for the add/remove/default cases,
and keep teamPayloadFromResult aligned with those required fields so missing
role becomes a compile-time error instead of rendering undefined.
- Around line 359-369: The rendering helpers are using payload.role without
guarding against it being missing, which can produce user-facing text like
undefined; update renderCoreTeamAddLines and apply the same fix in
renderCoreTeamRemoveLines and renderCoreTeamDefaultLines by either validating
role before interpolation or tightening the payload type so role is required,
and ensure the functions fail fast or handle the absence explicitly rather than
rendering an undefined value.
In `@src/daemon.ts`:
- Around line 1583-1653: The team mutation routes are missing the standard
mutation timeout, so update `teamInitTextRoute` and `teamRoleTextRoute` to pass
the same `{ timeoutMs: CLI_PROJECT_MUTATION_TIMEOUT_MS }` option used by other
mutation helpers in this file. Apply the change on the `postProjectServiceJson`
calls in those methods so `team.json` writes use the longer mutation timeout
instead of the default proxy timeout.
In `@src/installed-shim.test.ts`:
- Around line 745-774: The fallback test in the team daemon staleness case only
checks Node launcher invocation, so it can pass even if the health probe never
runs. Update this test to assert the health/status check occurred by inspecting
curlLog, similar to the matching-daemon test, alongside the existing run() and
nodeLog assertions in the makeFixture-based scenario.
In `@src/metadata-server.ts`:
- Around line 1282-1296: The fallback handling in removeTeamRole is using a
hardcoded "coder" default that can point to a deleted role. Update the
defaultRole reassignment in removeTeamRole to derive the fallback from
getDefaultTeamConfig().defaultRole or otherwise leave it unset when config.roles
becomes empty, and keep the logic aligned with loadTeamConfig/saveTeamConfig so
defaultRole always references an existing role.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63eb197b-7366-494d-beed-ab54d70fdb81
📒 Files selected for processing (15)
app/lib/api.tsapp/lib/project-api-route-coverage.tsdocs/command-ownership-inventory.mddocs/core-sidecar-north-star.mdscripts/installed-aimux-shim.shsrc/core-command-contract.tssrc/core-command-ownership.test.tssrc/core-text.tssrc/daemon.test.tssrc/daemon.tssrc/installed-shim.test.tssrc/main.tssrc/metadata-server.test.tssrc/metadata-server.tssrc/project-api-contract.ts
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Tests