Create GitHub teams in the background of Org requests #338
Conversation
💰 Infracost reportMonthly estimate generatedEstimate details (includes details of unsupported resources) |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
✅ Actions performedFull review triggered. |
WalkthroughGitHub team creation is refactored from synchronous in-route processing to asynchronous SQS queuing. The Changes
Sequence Diagram(s)sequenceDiagram
participant OrgRoute as Organization Route
participant SQS as SQS Queue
participant Handler as CreateOrgGithubTeam Handler
participant Redis as Redis Lock
participant DynamoDB as DynamoDB
participant GitHub as GitHub API
participant IDP as IDP (Entra)
rect rgb(240,248,255)
note over OrgRoute,SQS: Enqueue phase (sync)
OrgRoute->>SQS: Enqueue CreateOrgGithubTeam payload
OrgRoute-->>OrgRoute: Return response immediately
end
rect rgb(255,250,240)
note over SQS,Handler: Async processing
SQS->>Handler: Invoke handler with payload
Handler->>Redis: Acquire lock (with backoff)
Handler->>DynamoDB: Get org metadata
Handler->>Handler: Validate Entra group & no existing team
alt No pre-existing team
Handler->>GitHub: createGithubTeam(...) → { updated, id }
alt updated == true
Handler->>IDP: Optionally assign Entra group to team
Handler->>DynamoDB: TransactWrite (set teamId + audit log)
else updated == false
Handler->>DynamoDB: TransactWrite (audit only / skip teamId write)
end
else Pre-existing team
Handler-->>DynamoDB: Skip creation
end
Handler->>Redis: Release lock
Handler-->>SQS: Complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 6
🧹 Nitpick comments (6)
src/api/sqs/index.ts (1)
51-54: Optional: ensure logs reflect actual completion orderWe currently log “Finished” immediately after calling the handler without awaiting. Awaiting improves trace clarity and doesn’t change partial-batch semantics since the promise is already returned to allSettled.
- childLogger.info(`Starting handler for ${parsedBody.function}...`); - const result = func( + childLogger.info(`Starting handler for ${parsedBody.function}...`); + const result = await func( parsedBody.payload, parsedBody.metadata, childLogger, ); childLogger.info(`Finished handler for ${parsedBody.function}.`); return result;Also applies to: 87-95
src/api/functions/github.ts (1)
131-140: Annotate return type and consider more robust existence checks
- Add an explicit return type so the new shape is locked in at compile time.
- Optional: replace list-first-page check with pagination (or handle 422 conflict by fetching the existing team) to avoid duplicate-name races.
-export async function createGithubTeam({ +export type CreateGithubTeamResult = { updated: boolean; id: number }; +export async function createGithubTeam({ githubToken, orgId, parentTeamId, description, name, privacy, logger, -}: Omit<CreateGithubTeamInputs, "groupsToSync">) { +}: Omit<CreateGithubTeamInputs, "groupsToSync">): Promise<CreateGithubTeamResult> {Example existence strategy (optional):
// Prefer pagination to ensure we don't miss existing teams: const existingTeam = await octokit.paginate( "GET /orgs/{org}/teams", { org: orgId, per_page: 100 }, (response, acc) => acc.concat(response.data), ).then(list => list.find((t: any) => t.name === name));Also applies to: 155-156, 199-200
src/api/sqs/handlers/createOrgGithubTeam.ts (2)
132-145: Add a conditional update to avoid accidental overwrite.Protect the write if another process has already set a different team ID (rare but safer).
Update: { TableName: genericConfig.SigInfoTableName, Key: marshall({ primaryKey: `DEFINE#${orgName}`, entryId: "0", }), UpdateExpression: "SET leadsGithubTeamId = :githubTeamId, updatedAt = :updatedAt", + ConditionExpression: + "attribute_not_exists(leadsGithubTeamId) OR leadsGithubTeamId = :githubTeamId", ExpressionAttributeValues: marshall({ ":githubTeamId": teamId, ":updatedAt": new Date().toISOString(), }), },
94-115: Optional: consider idempotent IDP sync even when the team already exists.Current behavior skips sync when the team pre-exists, which could leave teams unmanaged if they were created manually. If safe, consider verifying existing mappings or performing an idempotent patch conditioned on GithubIdpSyncEnabled.
src/api/routes/organizations.ts (1)
575-591: Avoid queuing when external updates are disabled.You already compute shouldSkipEnhancedActions. Skip enqueuing to save SQS churn; the handler would skip anyway.
- // Queue creating GitHub team if needed - if (!githubTeamId) { + // Queue creating GitHub team if needed and allowed + if (!shouldSkipEnhancedActions && !githubTeamId) { const sqsPayload: SQSPayload<AvailableSQSFunctions.CreateOrgGithubTeam> = { function: AvailableSQSFunctions.CreateOrgGithubTeam, metadata: { initiator: request.username!, reqId: request.id, }, payload: { orgName: request.params.orgId, githubTeamDescription: grpDisplayName, githubTeamName: grpShortName, }, }; sqsPayloads.push(sqsPayload); }tests/unit/sqs/handlers/createOrgGithubTeam.test.ts (1)
212-240: Great coverage; consider making the IDP sync path deterministic.You note it’s hard to mock currentEnvironmentConfig. You can mock the module that exports it to force GithubIdpSyncEnabled = true/false and assert assignIdpGroupsToTeam calls precisely.
Example:
vi.mock("../../../../src/api/sqs/index.js", () => ({ currentEnvironmentConfig: { GithubIdpSyncEnabled: true, GithubOrgId: 123, GithubOrgName: "acm-uiuc", ExecGithubTeam: 1, GroupEmailSuffix: "", }, // export anything else needed by the handler... }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
src/api/functions/github.ts(2 hunks)src/api/package.json(2 hunks)src/api/routes/organizations.ts(3 hunks)src/api/sqs/handlers/createOrgGithubTeam.ts(1 hunks)src/api/sqs/handlers/index.ts(1 hunks)src/api/sqs/index.ts(2 hunks)src/common/types/sqsMessage.ts(3 hunks)tests/unit/functions/github.test.ts(3 hunks)tests/unit/organizations.test.ts(0 hunks)tests/unit/sqs/handlers/createOrgGithubTeam.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- tests/unit/organizations.test.ts
🧰 Additional context used
🪛 ESLint
src/api/sqs/handlers/index.ts
[error] 6-6: Unexpected use of file extension "js" for "./createOrgGithubTeam.js"
(import/extensions)
src/api/sqs/handlers/createOrgGithubTeam.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/JaNGFTBfxe'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)
(import/extensions)
[error] 1-1: Unexpected use of file extension "js" for "common/types/sqsMessage.js"
(import/extensions)
[error] 2-2: Unexpected use of file extension "js" for "../index.js"
(import/extensions)
[error] 8-8: Unexpected use of file extension "js" for "common/config.js"
(import/extensions)
[error] 9-9: Unexpected use of file extension "js" for "../utils.js"
(import/extensions)
[error] 13-13: Unexpected use of file extension "js" for "common/errors/index.js"
(import/extensions)
[error] 17-17: Unexpected use of file extension "js" for "api/functions/github.js"
(import/extensions)
[error] 18-18: Unexpected use of file extension "js" for "api/functions/auditLog.js"
(import/extensions)
[error] 19-19: Unexpected use of file extension "js" for "common/modules.js"
(import/extensions)
[error] 20-20: Unexpected use of file extension "js" for "api/utils.js"
(import/extensions)
[error] 21-21: Unexpected use of file extension "js" for "common/overrides.js"
(import/extensions)
tests/unit/sqs/handlers/createOrgGithubTeam.test.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/PCJvcLzPgx'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)
(import/extensions)
[error] 9-9: Unexpected use of file extension "js" for "../../../../src/api/sqs/handlers/createOrgGithubTeam.js"
(import/extensions)
[error] 10-10: Unexpected use of file extension "js" for "../../../../src/common/config.js"
(import/extensions)
[error] 11-11: Unexpected use of file extension "js" for "../../../../src/common/errors/index.js"
(import/extensions)
[error] 12-12: Unexpected use of file extension "js" for "../../../../src/common/overrides.js"
(import/extensions)
[error] 40-40: Unexpected use of file extension "js" for "../../../../src/api/functions/github.js"
(import/extensions)
[error] 267-267: Insert ⏎·········
(prettier/prettier)
src/common/types/sqsMessage.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/tRbbrFKyts'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)
(import/extensions)
[error] 1-1: Replace ·AllOrganizationNameList,·OrganizationsByName· with ⏎··AllOrganizationNameList,⏎··OrganizationsByName,⏎
(prettier/prettier)
[error] 75-75: Insert ⏎···
(prettier/prettier)
[error] 78-78: Insert ,
(prettier/prettier)
[error] 79-79: Insert ,
(prettier/prettier)
[error] 80-80: Insert ,
(prettier/prettier)
[error] 97-97: Insert ,
(prettier/prettier)
src/api/sqs/index.ts
[error] 25-25: Unexpected use of file extension "js" for "./handlers/createOrgGithubTeam.js"
(import/extensions)
src/api/routes/organizations.ts
[error] 57-57: Unexpected use of file extension "js" for "api/functions/sqs.js"
(import/extensions)
[error] 58-58: Unexpected use of file extension "js" for "api/utils.js"
(import/extensions)
[error] 62-62: Unexpected use of file extension "js" for "api/functions/github.js"
(import/extensions)
[error] 63-63: Unexpected use of file extension "js" for "common/overrides.js"
(import/extensions)
[error] 64-64: Unexpected use of file extension "js" for "common/types/sqsMessage.js"
(import/extensions)
🔇 Additional comments (4)
src/common/types/sqsMessage.ts (2)
83-90: Nice touch: exhaustiveness check for schemasThe AllSchemas helper that forces a schema per enum member is great for future-proofing. No changes requested.
1-1: Remove unused importOrganizationsByNameThe file has been properly formatted by Prettier, but ESLint flags that
OrganizationsByNameis imported but never used (line 3). Remove it from the import statement to pass CI.import { AllOrganizationNameList, - OrganizationsByName, } from "@acm-uiuc/js-shared";Likely an incorrect or invalid review comment.
src/api/package.json (1)
60-60: redlock-universal ^0.6.4 — verify actual package availability and test CIVersion availability is unclear: local npm registry shows 0.6.4 exists, but web sources report latest as 0.6.0. Verify:
npm installsucceeds and correct version is installed- CI build completes without resolution errors
- No breaking changes in SimpleLock type or createLock API by testing affected files (createOrgGithubTeam.ts, organizations.ts)
tests/unit/functions/github.test.ts (1)
56-56: Return-shape assertions look correct.The updates to expect { updated, id } are consistent with the new createGithubTeam API. Nothing else to change here.
Also applies to: 84-84, 160-160
| export { provisionNewMemberHandler } from "./provisionNewMember.js"; | ||
| export { sendSaleEmailHandler } from "./sendSaleEmailHandler.js"; | ||
| export { emailNotificationsHandler } from "./emailNotifications.js"; | ||
| export { createOrgGithubTeamHandler } from "./createOrgGithubTeam.js"; |
There was a problem hiding this comment.
Resolve ESLint “import/extensions” on re-export
Consistent with other exports in this file, the “.js” extension is needed for NodeNext ESM. If the rule is strict, silence it inline:
-export { createOrgGithubTeamHandler } from "./createOrgGithubTeam.js";
+export { createOrgGithubTeamHandler } from "./createOrgGithubTeam.js"; // eslint-disable-line import/extensionsAlternatively, relax the rule for TS files in .eslintrc when using "moduleResolution": "nodenext".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export { createOrgGithubTeamHandler } from "./createOrgGithubTeam.js"; | |
| export { createOrgGithubTeamHandler } from "./createOrgGithubTeam.js"; // eslint-disable-line import/extensions |
🧰 Tools
🪛 ESLint
[error] 6-6: Unexpected use of file extension "js" for "./createOrgGithubTeam.js"
(import/extensions)
🤖 Prompt for AI Agents
In src/api/sqs/handlers/index.ts around line 6, the re-export currently includes
the “.js” extension and ESLint's import/extensions rule is complaining; either
silence the rule inline for this line by adding an ESLint-disable comment (e.g.
// eslint-disable-next-line import/extensions) immediately above the export, or
update your project ESLint config to allow .js extensions in TS files when using
"moduleResolution": "nodenext" (adjust rules.import/extensions or override for
"*.ts" files); apply one of these fixes to keep the explicit .js re-export while
satisfying linting.
| import { ValidationError } from "../../common/errors/index.js"; | ||
| import { RunEnvironment } from "../../common/roles.js"; | ||
| import { environmentConfig } from "../../common/config.js"; | ||
| import { createOrgGithubTeamHandler } from "./handlers/createOrgGithubTeam.js"; |
There was a problem hiding this comment.
Fix ESLint import/extensions and consolidate handler import
ESLint flags the explicit “.js” on the direct handler import. To avoid the rule while keeping NodeNext-friendly imports, pull the symbol from the existing handlers barrel import and drop the extra line.
-import { createOrgGithubTeamHandler } from "./handlers/createOrgGithubTeam.js";
// ...
-import {
+import {
emailMembershipPassHandler,
pingHandler,
provisionNewMemberHandler,
sendSaleEmailHandler,
- emailNotificationsHandler,
+ emailNotificationsHandler,
+ createOrgGithubTeamHandler,
} from "./handlers/index.js";
// ...
[AvailableSQSFunctions.EmailNotifications]: emailNotificationsHandler,
[AvailableSQSFunctions.CreateOrgGithubTeam]: createOrgGithubTeamHandler,Additionally, consider awaiting the handler so the “Finished” log prints after completion:
- const result = func(
+ const result = await func(
parsedBody.payload,
parsedBody.metadata,
childLogger,
);Also applies to: 43-44
🧰 Tools
🪛 ESLint
[error] 25-25: Unexpected use of file extension "js" for "./handlers/createOrgGithubTeam.js"
(import/extensions)
🤖 Prompt for AI Agents
In src/api/sqs/index.ts around line 25 (and similarly lines 43-44), remove the
explicit handler import that includes the “.js” extension and instead import the
handler symbol from the existing handlers barrel (./handlers) to satisfy ESLint
import/extensions and NodeNext semantics; then when invoking the handler, await
its returned promise so the "Finished" log is emitted only after the handler
completes. Ensure you consolidate imports into the barrel import at the top and
replace the non-awaited call with an awaited call where the "Finished" log
follows.
| ), | ||
| [AvailableSQSFunctions.CreateOrgGithubTeam]: createSQSSchema( | ||
| AvailableSQSFunctions.CreateOrgGithubTeam, z.object({ | ||
| orgName: z.enum(AllOrganizationNameList), | ||
| githubTeamName: z.string().min(1), | ||
| githubTeamDescription: z.string().min(1) | ||
| }) | ||
| ) |
There was a problem hiding this comment.
Schema requires description, but downstream expects it optional
createGithubTeam builds [Managed by Core API] and only appends description if present. Making githubTeamDescription required here can reject valid calls and diverge from runtime expectations. Suggest making it optional and constraining lengths.
[AvailableSQSFunctions.CreateOrgGithubTeam]: createSQSSchema(
AvailableSQSFunctions.CreateOrgGithubTeam, z.object({
orgName: z.enum(AllOrganizationNameList),
- githubTeamName: z.string().min(1),
- githubTeamDescription: z.string().min(1)
+ githubTeamName: z.string().min(1).max(100),
+ githubTeamDescription: z.string().min(1).max(1000).optional()
})
)🧰 Tools
🪛 ESLint
[error] 75-75: Insert ⏎···
(prettier/prettier)
[error] 78-78: Insert ,
(prettier/prettier)
[error] 79-79: Insert ,
(prettier/prettier)
[error] 80-80: Insert ,
(prettier/prettier)
🤖 Prompt for AI Agents
In src/common/types/sqsMessage.ts around lines 73 to 80, the schema currently
requires githubTeamDescription but downstream code treats it as optional; change
the schema to make githubTeamDescription optional and add reasonable length
constraints (e.g., .trim().min(1).max(255).optional()) so valid calls without a
description are accepted while still enforcing non-empty and bounded length when
provided.
devksingh4
force-pushed
the
dsingh14/async-github-org-setup
branch
from
99c1281 to
0d3893c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/api/sqs/index.ts (2)
88-92: Await the handler invocation.The handler returns a Promise but is not awaited, causing the "Finished" log (line 93) to print before the handler completes. This can mislead operators about execution status.
25-25: Consolidate handler import into the barrel import.The direct import of
createOrgGithubTeamHandleris inconsistent with other handlers and triggers an ESLint violation. Import it from the existing handlers barrel at line 20-21 instead.src/api/sqs/handlers/index.ts (1)
6-6: ESLint import/extensions configuration mismatch.The
.jsextension is required for NodeNext ESM but conflicts with the import/extensions rule. This is consistent with other exports in this file and should be resolved at the ESLint configuration level rather than per-line.src/api/sqs/handlers/createOrgGithubTeam.ts (2)
50-58: Use ProjectionExpression instead of deprecated AttributesToGet.
AttributesToGetis deprecated in favor ofProjectionExpressionwithExpressionAttributeNames. Modern SDKs may reject or warn on deprecated parameters.
31-31: Redis client is never closed, causing connection leaks.A new Redis client is created on line 31 but never closed. In long-running Lambda workers or repeated invocations, this accumulates open connections.
src/common/types/sqsMessage.ts (1)
73-80: Schema requires description but downstream function accepts it as optional.The
githubTeamDescriptionfield is marked required (z.string().min(1)), butcreateGithubTeamin src/api/functions/github.ts (line 135) acceptsdescriptionas optional. This mismatch will reject valid SQS messages that omit the description.src/api/routes/organizations.ts (1)
417-419: Wrong attribute name causes unnecessary SQS enqueueing.Line 418 reads
githubTeamIdbut the DynamoDB attribute isleadsGithubTeamId(as shown in line 402). This will always beundefined, causing GitHub team creation to be enqueued even when a team already exists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
src/api/functions/github.ts(2 hunks)src/api/package.json(2 hunks)src/api/routes/organizations.ts(3 hunks)src/api/sqs/handlers/createOrgGithubTeam.ts(1 hunks)src/api/sqs/handlers/index.ts(1 hunks)src/api/sqs/index.ts(2 hunks)src/common/types/sqsMessage.ts(3 hunks)tests/unit/functions/github.test.ts(3 hunks)tests/unit/organizations.test.ts(0 hunks)tests/unit/sqs/handlers/createOrgGithubTeam.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- tests/unit/organizations.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/package.json
🧰 Additional context used
🪛 ESLint
src/api/sqs/index.ts
[error] 25-25: Unexpected use of file extension "js" for "./handlers/createOrgGithubTeam.js"
(import/extensions)
src/api/routes/organizations.ts
[error] 57-57: Unexpected use of file extension "js" for "api/functions/sqs.js"
(import/extensions)
[error] 58-58: Unexpected use of file extension "js" for "api/utils.js"
(import/extensions)
[error] 62-62: Unexpected use of file extension "js" for "api/functions/github.js"
(import/extensions)
[error] 63-63: Unexpected use of file extension "js" for "common/overrides.js"
(import/extensions)
[error] 64-64: Unexpected use of file extension "js" for "common/types/sqsMessage.js"
(import/extensions)
tests/unit/sqs/handlers/createOrgGithubTeam.test.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/YZtcIczsnP'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)
(import/extensions)
[error] 9-9: Unexpected use of file extension "js" for "../../../../src/api/sqs/handlers/createOrgGithubTeam.js"
(import/extensions)
[error] 10-10: Unexpected use of file extension "js" for "../../../../src/common/config.js"
(import/extensions)
[error] 11-11: Unexpected use of file extension "js" for "../../../../src/common/errors/index.js"
(import/extensions)
[error] 12-12: Unexpected use of file extension "js" for "../../../../src/common/overrides.js"
(import/extensions)
[error] 40-40: Unexpected use of file extension "js" for "../../../../src/api/functions/github.js"
(import/extensions)
[error] 267-267: Insert ⏎·········
(prettier/prettier)
src/api/sqs/handlers/createOrgGithubTeam.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/qngYBLCOzQ'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)
(import/extensions)
[error] 1-1: Unexpected use of file extension "js" for "common/types/sqsMessage.js"
(import/extensions)
[error] 2-2: Unexpected use of file extension "js" for "../index.js"
(import/extensions)
[error] 8-8: Unexpected use of file extension "js" for "common/config.js"
(import/extensions)
[error] 9-9: Unexpected use of file extension "js" for "../utils.js"
(import/extensions)
[error] 13-13: Unexpected use of file extension "js" for "common/errors/index.js"
(import/extensions)
[error] 17-17: Unexpected use of file extension "js" for "api/functions/github.js"
(import/extensions)
[error] 18-18: Unexpected use of file extension "js" for "api/functions/auditLog.js"
(import/extensions)
[error] 19-19: Unexpected use of file extension "js" for "common/modules.js"
(import/extensions)
[error] 20-20: Unexpected use of file extension "js" for "api/utils.js"
(import/extensions)
[error] 21-21: Unexpected use of file extension "js" for "common/overrides.js"
(import/extensions)
src/api/sqs/handlers/index.ts
[error] 6-6: Unexpected use of file extension "js" for "./createOrgGithubTeam.js"
(import/extensions)
src/common/types/sqsMessage.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/ApDIqBSXvF'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)
(import/extensions)
[error] 1-1: Replace ·AllOrganizationNameList,·OrganizationsByName· with ⏎··AllOrganizationNameList,⏎··OrganizationsByName,⏎
(prettier/prettier)
[error] 75-75: Insert ⏎···
(prettier/prettier)
[error] 78-78: Insert ,
(prettier/prettier)
[error] 79-79: Insert ,
(prettier/prettier)
[error] 80-80: Insert ,
(prettier/prettier)
[error] 97-97: Insert ,
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Application
- GitHub Check: Run Unit Tests
🔇 Additional comments (7)
tests/unit/functions/github.test.ts (1)
56-56: LGTM!Test expectations correctly updated to validate the new return shape
{ updated: boolean; id: number }fromcreateGithubTeam, covering both existing team and new team scenarios.Also applies to: 84-84, 160-160
src/api/functions/github.ts (1)
155-155: LGTM!The enhanced return type
{ updated: boolean; id: number }allows callers to distinguish between newly created and pre-existing teams, enabling conditional downstream logic like IDP sync setup.Also applies to: 199-199
src/common/types/sqsMessage.ts (1)
83-89: LGTM!The exhaustiveness check using TypeScript's type system ensures all enum members have corresponding schemas, preventing missing handler registrations at compile time.
src/api/routes/organizations.ts (1)
569-591: LGTM!The SQS-based async flow cleanly decouples GitHub team creation from the HTTP request path, improving response time and resilience. The conditional enqueueing logic and payload construction are correct.
tests/unit/sqs/handlers/createOrgGithubTeam.test.ts (1)
1-363: LGTM!Comprehensive test coverage including edge cases (disabled orgs, missing Entra group, existing teams), happy paths (new team creation, IDP sync), transactional behavior (audit logging), and error scenarios (lock loss). The tests properly validate the
updatedflag to distinguish new vs existing teams.src/api/sqs/handlers/createOrgGithubTeam.ts (2)
94-115: LGTM!The conditional IDP sync setup (lines 102-114) based on the
updatedflag prevents redundant API calls when a team already exists, improving efficiency and avoiding potential errors from re-syncing.
117-126: LGTM!Conditional audit logging ensures only actual team creation events are recorded, preventing misleading audit trails for no-op operations.
| const currentOrgInfo = unmarshall(existingData.Item) as { | ||
| leadsEntraGroupId?: string; | ||
| leadsGithubTeamId?: string; | ||
| }; |
There was a problem hiding this comment.
Type mismatch: leadsGithubTeamId should be number, not string.
Line 67 types leadsGithubTeamId as string but it stores a numeric GitHub team ID. Line 73 compares it in a boolean context and line 141 assigns a number to it. This should be typed as number | undefined.
Apply this fix:
const currentOrgInfo = unmarshall(existingData.Item) as {
leadsEntraGroupId?: string;
- leadsGithubTeamId?: string;
+ leadsGithubTeamId?: number;
};🤖 Prompt for AI Agents
In src/api/sqs/handlers/createOrgGithubTeam.ts around lines 65 to 68, the
unmarshall cast types leadsGithubTeamId as a string but the code stores and uses
numeric GitHub team IDs; change the cast to use leadsGithubTeamId?: number
(i.e., number | undefined). Update any boolean checks that rely on string
truthiness to explicitly check for undefined/null (e.g., if
(currentOrgInfo.leadsGithubTeamId != null) or if (typeof
currentOrgInfo.leadsGithubTeamId === 'number')), and ensure assignments write a
number value (no string conversion).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/routes/organizations.ts (1)
396-404: Replace deprecated AttributesToGet with ProjectionExpression.AttributesToGet is deprecated and can be rejected; use ProjectionExpression + ExpressionAttributeNames.
const getMetadataCommand = new GetItemCommand({ TableName: genericConfig.SigInfoTableName, Key: marshall({ primaryKey: `DEFINE#${request.params.orgId}`, entryId: "0", }), - AttributesToGet: ["leadsEntraGroupId", "leadsGithubTeamId"], + ProjectionExpression: "#entra,#gh", + ExpressionAttributeNames: { + "#entra": "leadsEntraGroupId", + "#gh": "leadsGithubTeamId", + }, ConsistentRead: true, });
♻️ Duplicate comments (4)
src/api/sqs/handlers/createOrgGithubTeam.ts (3)
51-63: Good: moved to ProjectionExpression.
162-168: Good: Redis client cleanup with try/finally.Prevents socket leaks in long‑lived workers.
70-73: Type correctness: leadsGithubTeamId should be a number.The codebase confirms this field is stored and used as a numeric GitHub team ID. The test file shows
leadsGithubTeamId: 456(number),organizations.tsexplicitly casts tonumberwhen reading it, andcreateOrgGithubTeam.tswrites numericteamIdto this field. The string type annotation at line 72 is incorrect.const currentOrgInfo = unmarshall(existingData.Item) as { leadsEntraGroupId?: string; - leadsGithubTeamId?: string; + leadsGithubTeamId?: number; };src/api/routes/organizations.ts (1)
417-419: Good: fixed attribute name to leadsGithubTeamId.
🧹 Nitpick comments (3)
src/api/routes/organizations.ts (3)
569-573: SQS client lazy init is fine.Consider centralizing client creation to reuse across routes, but this is acceptable.
575-591: Optionally gate enqueue for skip set to avoid no‑op SQS work.Handler already skips based on SKIP_EXTERNAL_ORG_LEAD_UPDATE; you can avoid pushing messages that will be dropped.
- if (!githubTeamId) { + if (!githubTeamId && !shouldSkipEnhancedActions) { const sqsPayload: SQSPayload<AvailableSQSFunctions.CreateOrgGithubTeam> = { function: AvailableSQSFunctions.CreateOrgGithubTeam, metadata: { initiator: request.username!, reqId: request.id }, payload: { orgName: request.params.orgId, githubTeamDescription: grpDisplayName, githubTeamName: grpShortName, }, }; sqsPayloads.push(sqsPayload); }
56-64: ESM import extensions vs ESLint import/extensions rule.These .js extensions are expected with TS NodeNext ESM. If ESLint flags them, update your import/extensions and resolver config (eslint-plugin-import) to allow .js for TS ESM, or disable the rule for this project.
Would you like a PR-ready eslint config snippet to align this?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/api/routes/organizations.ts(3 hunks)src/api/sqs/handlers/createOrgGithubTeam.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
src/api/sqs/handlers/createOrgGithubTeam.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/stjLCaDxAv'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)
(import/extensions)
[error] 1-1: Unexpected use of file extension "js" for "common/types/sqsMessage.js"
(import/extensions)
[error] 2-2: Unexpected use of file extension "js" for "../index.js"
(import/extensions)
[error] 8-8: Unexpected use of file extension "js" for "common/config.js"
(import/extensions)
[error] 9-9: Unexpected use of file extension "js" for "../utils.js"
(import/extensions)
[error] 13-13: Unexpected use of file extension "js" for "common/errors/index.js"
(import/extensions)
[error] 17-17: Unexpected use of file extension "js" for "api/functions/github.js"
(import/extensions)
[error] 18-18: Unexpected use of file extension "js" for "api/functions/auditLog.js"
(import/extensions)
[error] 19-19: Unexpected use of file extension "js" for "common/modules.js"
(import/extensions)
[error] 20-20: Unexpected use of file extension "js" for "api/utils.js"
(import/extensions)
[error] 21-21: Unexpected use of file extension "js" for "common/overrides.js"
(import/extensions)
src/api/routes/organizations.ts
[error] 57-57: Unexpected use of file extension "js" for "api/functions/sqs.js"
(import/extensions)
[error] 58-58: Unexpected use of file extension "js" for "api/utils.js"
(import/extensions)
[error] 62-62: Unexpected use of file extension "js" for "api/functions/github.js"
(import/extensions)
[error] 63-63: Unexpected use of file extension "js" for "common/overrides.js"
(import/extensions)
[error] 64-64: Unexpected use of file extension "js" for "common/types/sqsMessage.js"
(import/extensions)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Unit Tests
- GitHub Check: Build Application
🔇 Additional comments (1)
src/api/sqs/handlers/createOrgGithubTeam.ts (1)
44-50: Lock TTL: ensure it exceeds worst‑case GitHub/IDP latency.Default durations can be short; long GitHub calls may outlive the lease and trip the aborted check. Set an explicit, conservative TTL (e.g., 60–120s) per your redlock‑universal API, and consider jitter.
Can you confirm the current default TTL and update createLock options accordingly?
| const { orgName, githubTeamName, githubTeamDescription } = payload; | ||
| const orgImmutableId = getOrgByName(orgName)!.id; | ||
| if (SKIP_EXTERNAL_ORG_LEAD_UPDATE.includes(orgImmutableId)) { | ||
| logger.info( | ||
| `Organization ${orgName} has external updates disabled, exiting.`, | ||
| ); | ||
| return; | ||
| } | ||
| const dynamo = new DynamoDBClient({ |
There was a problem hiding this comment.
Avoid non-null assertion on getOrgByName; harden against malformed payloads.
A bad SQS payload would crash the worker. Guard and emit a clear error.
- const { orgName, githubTeamName, githubTeamDescription } = payload;
- const orgImmutableId = getOrgByName(orgName)!.id;
+ const { orgName, githubTeamName, githubTeamDescription } = payload;
+ const orgInfo = getOrgByName(orgName);
+ if (!orgInfo) {
+ logger.warn({ orgName }, "Unknown organization; skipping GitHub team creation.");
+ return;
+ }
+ const orgImmutableId = orgInfo.id;🤖 Prompt for AI Agents
In src/api/sqs/handlers/createOrgGithubTeam.ts around lines 33-41, the code uses
a non-null assertion on getOrgByName which will crash if the SQS payload is
malformed; replace the assertion with a safe lookup: first validate
payload.orgName exists, call getOrgByName(orgName) into a variable, check if the
result is undefined, and if so log a clear error including the payload/orgName
and exit early (or route to DLQ) instead of proceeding; only when org is found
read org.id and continue with the SKIP_EXTERNAL_ORG_LEAD_UPDATE check.
1 participant
Summary by CodeRabbit
Refactor
Chores
Tests