[comp] Production Deploy#2878
Open
github-actions[bot] wants to merge 23 commits into
Open
Conversation
feat(people): add employment events tracking and offboarding checklist
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
20 issues found across 55 files
Confidence score: 2/5
- Several high-confidence backend issues are concrete and user-impacting (8/10), including unsanitized ZIP entry filenames in
apps/api/src/offboarding-checklist/offboarding-export.service.tsand unscoped tenant/member deletes inapps/api/src/offboarding-checklist/access-revocation.service.tsandapps/api/src/people/people.controller.ts. - There is meaningful data integrity risk in
apps/api/src/offboarding-checklist/offboarding-checklist.service.ts:completeItemcan remain completed when evidence upload fails, leaving partial state that should be rolled back. - Additional medium issues (6–7/10) in checklist/export flows suggest regression potential rather than just style concerns, such as CSV formula injection handling and pending-offboarding count mismatches in
apps/api/src/offboarding-checklist/offboarding-export.service.tsandapps/api/src/offboarding-checklist/offboarding-checklist.service.ts. - Pay close attention to
apps/api/src/offboarding-checklist/offboarding-export.service.ts,apps/api/src/offboarding-checklist/access-revocation.service.ts,apps/api/src/people/people.controller.ts,apps/api/src/offboarding-checklist/offboarding-checklist.service.ts- security scoping, path/input sanitization, and transactional consistency need tightening before merge.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/AccessRevocationList.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/AccessRevocationList.tsx:52">
P2: Using a single `processingVendorId` causes incorrect loading/disabled behavior when multiple row actions overlap. Track processing per vendor (or globally lock actions) to avoid prematurely re-enabling controls during in-flight requests.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/people/settings/components/OffboardingChecklistSettings.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/people/settings/components/OffboardingChecklistSettings.tsx:57">
P2: Rolling back with a full-list snapshot can clobber other in-flight item updates. Revert only the failed item field (or revalidate from server) instead of restoring `data: previous`.</violation>
</file>
<file name="apps/api/src/offboarding-checklist/offboarding-checklist.controller.ts">
<violation number="1" location="apps/api/src/offboarding-checklist/offboarding-checklist.controller.ts:251">
P2: Use a DTO with class-validator rules for `revokeVendorAccess` body instead of an inline type, and enforce all-or-none evidence fields with a 400 response on partial payloads.</violation>
</file>
<file name="apps/api/src/offboarding-checklist/offboarding-checklist.service.ts">
<violation number="1" location="apps/api/src/offboarding-checklist/offboarding-checklist.service.ts:49">
P2: `createTemplateItem` has a race condition for `sortOrder`; concurrent creates can assign the same order value.</violation>
</file>
<file name="apps/app/src/hooks/use-access-revocations.ts">
<violation number="1" location="apps/app/src/hooks/use-access-revocations.ts:49">
P2: Add a file-size check before base64 conversion to avoid loading oversized evidence files into browser memory.</violation>
</file>
<file name="apps/api/src/offboarding-checklist/access-revocation.service.ts">
<violation number="1" location="apps/api/src/offboarding-checklist/access-revocation.service.ts:204">
P2: Avoid calling the full `getAccessRevocations` path in completion sync; it does expensive attachment fetches that are unnecessary for count checks.</violation>
</file>
<file name="apps/app/src/hooks/use-employment-evidence.ts">
<violation number="1" location="apps/app/src/hooks/use-employment-evidence.ts:32">
P2: Add a client-side file size check before `readAsDataURL` to avoid loading oversized files into memory before the server rejects them.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
|
||
| const uploadEvidence = useCallback( | ||
| (file: File): Promise<AttachmentMetadata> => { | ||
| return new Promise((resolve, reject) => { |
Contributor
There was a problem hiding this comment.
P2: Add a client-side file size check before readAsDataURL to avoid loading oversized files into memory before the server rejects them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/hooks/use-employment-evidence.ts, line 32:
<comment>Add a client-side file size check before `readAsDataURL` to avoid loading oversized files into memory before the server rejects them.</comment>
<file context>
@@ -0,0 +1,95 @@
+
+ const uploadEvidence = useCallback(
+ (file: File): Promise<AttachmentMetadata> => {
+ return new Promise((resolve, reject) => {
+ const reader = new FileReader();
+ reader.onload = async () => {
</file context>
Contributor
There was a problem hiding this comment.
Won't fix — file size is validated server-side. Adding a client-side check is a nice-to-have but doesn't prevent any actual issue since the server rejects oversized payloads. The file input accept attribute already limits to expected document types.
Contributor
There was a problem hiding this comment.
Thanks for the clarification—understood that server-side validation handles file size here.
* feat(frameworks): show controls as default tab with requirement column Users expect to see controls first when clicking into a framework. Adds a Controls tab (default) showing all controls with their mapped requirement identifiers, keeping Requirements as a secondary tab. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(people): use valid badge variant for critical offboarding items Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Mariano Fuentes <marfuen98@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(people): ds component compatibility fixes for offboarding UI - remove redundant wrapper divs from PopoverContent (DS already renders card chrome) - remove className from PopoverContent (not accepted by DS) - use render prop on DropdownMenuTrigger to avoid nested buttons - use accent variant for Critical badge instead of destructive Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(people): override popover width for date range filter DS PopoverContent has hardcoded w-72 which clips the 380px filter content. Use inline style to override width to auto. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Mariano Fuentes <marfuen98@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(evidence-export): load automations one at a time to prevent OOM The export endpoint loaded all automation runs with full results and logs into memory simultaneously, causing V8 heap OOM at ~6 GB on large orgs. Now loads lightweight headers first, then each automation's data individually so peak memory ≈ one automation instead of all combined. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(evidence-export): paginate runs, extract mappers, eliminate duplication - Paginate loadFullAutomation in batches of 50 runs to bound JSON.parse pressure for enormous automations - Extract Prisma→normalizer mappers (toAppAutomationRun, toCustomAutomationRun) as single source of truth — eliminates duplicated mapping in service - Rewrite getTaskEvidenceSummary to reuse data loader instead of duplicating 100 lines of query + mapping code - Extract buildAutomationJson to evidence-json-builder.ts - Service: 600 → 411 lines Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(evidence-export): add deterministic tiebreaker to paginated queries Offset pagination ordered only by createdAt could skip or duplicate rows when multiple runs share the same timestamp. Adding id as a secondary sort key makes the ordering deterministic across batches. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(evidence-export): use cursor pagination instead of offset Offset pagination (skip) can duplicate or miss rows when new automation runs are inserted during a long export. Cursor-based pagination uses keyset lookups (WHERE id < cursor) which are stable under concurrent writes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Mariano Fuentes <marfuen98@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…callers
A customer (Josh) hit a 401 calling
POST /v1/cloud-security/findings/:id/exception with an X-API-Key:
"Marking an exception requires session authentication." The same
inline check existed on revokeException and updateAwsScanMode. With
MCP integration coming soon we want the full cloud-tests mutation
surface to be API-callable.
The reason the wall existed: AuditLog.userId + FindingException.markedById
are both String/FK to User, and API keys are org-scoped (no per-user
identity). This change introduces a shared ActingUserResolver that
attributes API key / service-token mutations to the org's oldest
owner-role member — same pattern 19+ other services in the codebase
already use for owner lookups. The audit log description prepends
`[via API key "<name>"]` so reviewers see at a glance that it was
automation, not a UI action.
What's preserved:
- Authentication unchanged (HybridAuthGuard runs as before).
- RBAC unchanged (PermissionGuard runs as before — caller still
needs integration:update scope).
- Cross-tenant isolation preserved (owner lookup scoped to the
calling org).
- Session callers: zero behavior change — resolver short-circuits
to req.userId with NO DB query. Audit description unchanged
(no [via ...] prefix).
- Existing API key writes (vendor:create, risk:create, etc.):
untouched.
- Service tokens passing x-user-id: unchanged (HybridAuthGuard
already sets req.userId, resolver short-circuits).
What changes:
- Service tokens without x-user-id: now succeed with owner-fallback
attribution (previously hit the inline 401 too).
- API key callers: now succeed with owner-fallback attribution.
- Org with no owner-role member: 400 with actionable message
("ensure your organization has at least one user with the owner
role"). Soft failure, never a 500.
Tests:
- 10 acting-user.service tests (session short-circuit, owner
fallback, cross-tenant scoping, deterministic oldest-owner pick,
no-owner soft failure, caller-label formatting)
- 12 exception.service tests (+2 for callerLabel propagation —
audit description prefix + metadata field)
- 5 NEW aws-scan-mode.service tests (idempotent no-op, AWS-only
enforcement, owner-fallback callerLabel)
- 9 NEW controller-level tests covering all 3 endpoints × 3
scenarios (session, API-key+owner, no-owner → 400)
- 253 cloud-security + auth tests pass (3 pre-existing TLS-env
failures unrelated — same suites fail on main)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ations feat(api): unblock cloud-tests mutations for API key + service token callers
7 tasks
…2884) - prevent CSV formula injection in export by prefixing dangerous chars - sanitize filenames in ZIP paths to prevent path traversal - add member ID to export folder names to prevent collisions - scope revocation lookup by organizationId to prevent cross-tenant access - rollback completion record if evidence upload fails - filter completion counts to enabled template items only - use @ISINT() for sortOrder DTO validation - validate date query params before building DB filters - scope attachment deletion to member and event type - show "Until {date}" when only end date is set in filter - clear file input in finally block for retry support - validate tab param against dynamically available tabs - show error state in TodosOverview on fetch failure Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
@cubic-dev-ai re-review this |
Contributor
@Marfuen I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
16 issues found across 82 files
Confidence score: 2/5
- There are high-confidence, high-severity data-integrity risks in
apps/api/src/offboarding-checklist/access-revocation.service.ts:revokeAllVendorAccesscan create cross-tenant revocation rows ifmemberIdis not verified againstorganizationIdbeforecreateMany. - Auth handling appears brittle in write paths (
apps/api/src/offboarding-checklist/offboarding-checklist.controller.ts,apps/api/src/people/people.controller.ts), where missingauthContext.userIdunder API key/service-token flows can cause runtime failures or unattributed uploads. - This is scored as high merge risk because the issues are concrete and user-impacting (tenant boundary violations, audit/history integrity concerns, and failing tests in
apps/api/src/offboarding-checklist/offboarding-checklist.service.spec.tsdue tofindUniquevsfindFirstmocking mismatch). - Pay close attention to
apps/api/src/offboarding-checklist/access-revocation.service.ts,apps/api/src/offboarding-checklist/offboarding-checklist.controller.ts,apps/api/src/people/people.controller.ts,packages/db/prisma/schema/offboarding-checklist.prisma- tenant scoping, actor attribution, and audit-history retention need tightening before merge.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/AccessRevocationList.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/AccessRevocationList.tsx:52">
P2: Using a single `processingVendorId` causes incorrect loading/disabled behavior when multiple row actions overlap. Track processing per vendor (or globally lock actions) to avoid prematurely re-enabling controls during in-flight requests.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/people/settings/components/OffboardingChecklistSettings.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/people/settings/components/OffboardingChecklistSettings.tsx:57">
P2: Rolling back with a full-list snapshot can clobber other in-flight item updates. Revert only the failed item field (or revalidate from server) instead of restoring `data: previous`.</violation>
</file>
<file name="apps/api/src/offboarding-checklist/offboarding-checklist.service.ts">
<violation number="1" location="apps/api/src/offboarding-checklist/offboarding-checklist.service.ts:49">
P2: `createTemplateItem` has a race condition for `sortOrder`; concurrent creates can assign the same order value.</violation>
</file>
<file name="apps/app/src/hooks/use-access-revocations.ts">
<violation number="1" location="apps/app/src/hooks/use-access-revocations.ts:49">
P2: Add a file-size check before base64 conversion to avoid loading oversized evidence files into browser memory.</violation>
</file>
<file name="apps/app/src/hooks/use-employment-evidence.ts">
<violation number="1" location="apps/app/src/hooks/use-employment-evidence.ts:32">
P2: Add a client-side file size check before `readAsDataURL` to avoid loading oversized files into memory before the server rejects them.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/components/FrameworkControls.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/components/FrameworkControls.tsx:177">
P2: This per-row computation repeatedly scans tasks and re-sorts evidence submissions, causing avoidable O(n×m) render-time work for larger control lists.</violation>
</file>
<file name="apps/api/src/cloud-security/ai-remediation.prompt.ts">
<violation number="1" location="apps/api/src/cloud-security/ai-remediation.prompt.ts:182">
P2: The prompt mixes IAM action syntax with SDK command-class syntax (`iam:CreateServiceLinkedRoleCommand`), which can make generated fix steps use an invalid command name.</violation>
</file>
<file name="apps/api/src/cloud-security/plan-normalizer.ts">
<violation number="1" location="apps/api/src/cloud-security/plan-normalizer.ts:59">
P2: Whitespace-only `AWSServiceName` values are treated as valid, so malformed SLR steps can bypass normalization and still fail at execution time.</violation>
</file>
<file name="apps/api/src/cloud-security/aws-command-executor.ts">
<violation number="1" location="apps/api/src/cloud-security/aws-command-executor.ts:411">
P2: Trailing space in `'invalidparameter '` will miss error messages where "InvalidParameter" is followed by punctuation or appears at the end of the string. Since `'invalidparametervalue'` is already matched by an earlier check (short-circuits via `||`), you can safely use `'invalidparameter'` without the space—or add a word-boundary-style check.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| return steps.map((step, idx) => { | ||
| if (step.command !== SLR_COMMAND) return step; | ||
| const existing = step.params?.AWSServiceName; | ||
| if (typeof existing === 'string' && existing.length > 0) return step; |
Contributor
There was a problem hiding this comment.
P2: Whitespace-only AWSServiceName values are treated as valid, so malformed SLR steps can bypass normalization and still fail at execution time.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/cloud-security/plan-normalizer.ts, line 59:
<comment>Whitespace-only `AWSServiceName` values are treated as valid, so malformed SLR steps can bypass normalization and still fail at execution time.</comment>
<file context>
@@ -0,0 +1,95 @@
+ return steps.map((step, idx) => {
+ if (step.command !== SLR_COMMAND) return step;
+ const existing = step.params?.AWSServiceName;
+ if (typeof existing === 'string' && existing.length > 0) return step;
+ const inferred = inferServiceLinkedRolePrincipal(steps, idx);
+ if (!inferred) return step;
</file context>
| lower.includes('failed to satisfy constraint') || | ||
| lower.includes('member must not be null') || | ||
| lower.includes('invalidparametervalue') || | ||
| lower.includes('invalidparameter ') || |
Contributor
There was a problem hiding this comment.
P2: Trailing space in 'invalidparameter ' will miss error messages where "InvalidParameter" is followed by punctuation or appears at the end of the string. Since 'invalidparametervalue' is already matched by an earlier check (short-circuits via ||), you can safely use 'invalidparameter' without the space—or add a word-boundary-style check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/cloud-security/aws-command-executor.ts, line 411:
<comment>Trailing space in `'invalidparameter '` will miss error messages where "InvalidParameter" is followed by punctuation or appears at the end of the string. Since `'invalidparametervalue'` is already matched by an earlier check (short-circuits via `||`), you can safely use `'invalidparameter'` without the space—or add a word-boundary-style check.</comment>
<file context>
@@ -372,6 +390,32 @@ function isValidationError(errName: string, errMsg: string): boolean {
+ lower.includes('failed to satisfy constraint') ||
+ lower.includes('member must not be null') ||
+ lower.includes('invalidparametervalue') ||
+ lower.includes('invalidparameter ') ||
+ lower.includes('invalid parameter') ||
+ lower.includes('must be a valid') ||
</file context>
Suggested change
| lower.includes('invalidparameter ') || | |
| (lower.includes('invalidparameter') && !lower.includes('invalidparametervalue')) || |
5 tasks
- fix test mock: findUnique → findFirst for undoVendorRevocation - guard uploadEmploymentEvidence against missing userId in API key auth - change User onDelete to SetNull on completions/revocations (preserve audit) - add requireUserId helper to offboarding controller for all mutations - validate member belongs to org before revocation operations - validate provider date strings before converting to Date in sync - reject partial evidence payloads in revokeVendorAccess - add noopener,noreferrer to all window.open calls - disable attach evidence button while processing - handle error state explicitly in OffboardingBanner Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
@cubic-dev-ai re-review this |
Contributor
@Marfuen I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
12 issues found across 82 files
Confidence score: 3/5
- There is concrete user-impacting risk:
apps/api/src/offboarding-checklist/offboarding-export.service.tscan produce non-unique evidence filenames, which may cause ZIP extraction collisions and evidence loss. apps/api/src/offboarding-checklist/offboarding-checklist.service.spec.tsreferences an undefinedfindFirstmock in a success test, which is likely to fail tests and reduce confidence in this change set until fixed.- Several medium-severity consistency issues add regression risk (non-atomic revoke flow in
apps/api/src/offboarding-checklist/access-revocation.service.ts, stale rollback state inapps/app/src/app/(app)/[orgId]/people/settings/components/OffboardingChecklistSettings.tsx, and incomplete repair equality checks inapps/api/src/cloud-security/aws-command-executor.ts). - Pay close attention to
apps/api/src/offboarding-checklist/offboarding-export.service.ts,apps/api/src/offboarding-checklist/access-revocation.service.ts,apps/app/src/app/(app)/[orgId]/people/settings/components/OffboardingChecklistSettings.tsx- these areas have the highest chance of user-visible data or state inconsistencies.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/AccessRevocationList.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/AccessRevocationList.tsx:52">
P2: Using a single `processingVendorId` causes incorrect loading/disabled behavior when multiple row actions overlap. Track processing per vendor (or globally lock actions) to avoid prematurely re-enabling controls during in-flight requests.</violation>
</file>
<file name="apps/api/src/offboarding-checklist/offboarding-checklist.service.ts">
<violation number="1" location="apps/api/src/offboarding-checklist/offboarding-checklist.service.ts:49">
P2: `createTemplateItem` has a race condition for `sortOrder`; concurrent creates can assign the same order value.</violation>
</file>
<file name="apps/app/src/hooks/use-access-revocations.ts">
<violation number="1" location="apps/app/src/hooks/use-access-revocations.ts:49">
P2: Add a file-size check before base64 conversion to avoid loading oversized evidence files into browser memory.</violation>
</file>
<file name="apps/app/src/hooks/use-employment-evidence.ts">
<violation number="1" location="apps/app/src/hooks/use-employment-evidence.ts:32">
P2: Add a client-side file size check before `readAsDataURL` to avoid loading oversized files into memory before the server rejects them.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/components/FrameworkControls.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/components/FrameworkControls.tsx:177">
P2: This per-row computation repeatedly scans tasks and re-sorts evidence submissions, causing avoidable O(n×m) render-time work for larger control lists.</violation>
</file>
<file name="apps/api/src/cloud-security/ai-remediation.prompt.ts">
<violation number="1" location="apps/api/src/cloud-security/ai-remediation.prompt.ts:182">
P2: The prompt mixes IAM action syntax with SDK command-class syntax (`iam:CreateServiceLinkedRoleCommand`), which can make generated fix steps use an invalid command name.</violation>
<violation number="2" location="apps/api/src/cloud-security/ai-remediation.prompt.ts:182">
P2: Use the AWS SDK command class name without the `iam:` prefix to avoid generating invalid `command` values.</violation>
</file>
<file name="apps/api/src/cloud-security/plan-normalizer.ts">
<violation number="1" location="apps/api/src/cloud-security/plan-normalizer.ts:59">
P2: Whitespace-only `AWSServiceName` values are treated as valid, so malformed SLR steps can bypass normalization and still fail at execution time.</violation>
</file>
<file name="apps/api/src/cloud-security/aws-command-executor.ts">
<violation number="1" location="apps/api/src/cloud-security/aws-command-executor.ts:411">
P2: Trailing space in `'invalidparameter '` will miss error messages where "InvalidParameter" is followed by punctuation or appears at the end of the string. Since `'invalidparametervalue'` is already matched by an earlier check (short-circuits via `||`), you can safely use `'invalidparameter'` without the space—or add a word-boundary-style check.</violation>
<violation number="2" location="apps/api/src/cloud-security/aws-command-executor.ts:740">
P2: Repair equality check only compares `params`, ignoring potential `service`/`command` changes from the AI callback. If the repair function fixes the command name (e.g., correcting `PutBucketPolicyCommand` → `PutBucketEncryptionCommand`) but keeps similar params, the fix is silently discarded.
Compare the full step instead of just params.</violation>
</file>
<file name="apps/app/src/hooks/use-offboarding-checklist.ts">
<violation number="1" location="apps/app/src/hooks/use-offboarding-checklist.ts:63">
P2: Add a file-size limit check before base64 conversion to avoid high client memory usage on large evidence uploads.</violation>
</file>
<file name="apps/api/src/people/people.controller.ts">
<violation number="1" location="apps/api/src/people/people.controller.ts:627">
P2: This delete path calls `getAttachments`, which unnecessarily generates signed URLs for every attachment even though only IDs are used for existence checking.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| - Service-linked roles (GuardDuty, Config, Inspector, Macie): use CreateServiceLinkedRole — AWS manages them. | ||
|
|
||
| ## SERVICE-LINKED ROLE AWSServiceName VALUES (MANDATORY) | ||
| When emitting iam:CreateServiceLinkedRoleCommand you MUST populate the |
Contributor
There was a problem hiding this comment.
P2: Use the AWS SDK command class name without the iam: prefix to avoid generating invalid command values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/cloud-security/ai-remediation.prompt.ts, line 182:
<comment>Use the AWS SDK command class name without the `iam:` prefix to avoid generating invalid `command` values.</comment>
<file context>
@@ -178,6 +178,20 @@ A human will ALWAYS review your plan before execution. Be precise and correct.
- Service-linked roles (GuardDuty, Config, Inspector, Macie): use CreateServiceLinkedRole — AWS manages them.
+## SERVICE-LINKED ROLE AWSServiceName VALUES (MANDATORY)
+When emitting iam:CreateServiceLinkedRoleCommand you MUST populate the
+AWSServiceName param. Leaving it null or empty causes AWS to reject the
+call with "Member must not be null". Use exactly these values:
</file context>
Suggested change
| When emitting iam:CreateServiceLinkedRoleCommand you MUST populate the | |
| When emitting CreateServiceLinkedRoleCommand you MUST populate the |
Comment on lines
+740
to
+741
| JSON.stringify(refined.params ?? {}) !== | ||
| JSON.stringify(originalStep.params ?? {}) |
Contributor
There was a problem hiding this comment.
P2: Repair equality check only compares params, ignoring potential service/command changes from the AI callback. If the repair function fixes the command name (e.g., correcting PutBucketPolicyCommand → PutBucketEncryptionCommand) but keeps similar params, the fix is silently discarded.
Compare the full step instead of just params.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/cloud-security/aws-command-executor.ts, line 740:
<comment>Repair equality check only compares `params`, ignoring potential `service`/`command` changes from the AI callback. If the repair function fixes the command name (e.g., correcting `PutBucketPolicyCommand` → `PutBucketEncryptionCommand`) but keeps similar params, the fix is silently discarded.
Compare the full step instead of just params.</comment>
<file context>
@@ -607,22 +687,91 @@ export async function executePlanSteps(params: {
+ });
+ if (
+ refined &&
+ JSON.stringify(refined.params ?? {}) !==
+ JSON.stringify(originalStep.params ?? {})
+ ) {
</file context>
Suggested change
| JSON.stringify(refined.params ?? {}) !== | |
| JSON.stringify(originalStep.params ?? {}) | |
| JSON.stringify(refined) !== | |
| JSON.stringify(originalStep) |
Comment on lines
+627
to
+631
| const attachments = await this.attachmentsService.getAttachments( | ||
| organizationId, | ||
| memberId, | ||
| entityType, | ||
| ); |
Contributor
There was a problem hiding this comment.
P2: This delete path calls getAttachments, which unnecessarily generates signed URLs for every attachment even though only IDs are used for existence checking.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/people/people.controller.ts, line 627:
<comment>This delete path calls `getAttachments`, which unnecessarily generates signed URLs for every attachment even though only IDs are used for existence checking.</comment>
<file context>
@@ -529,6 +565,77 @@ export class PeopleController {
+ ) {
+ const entityType = this.resolveEventType(eventType);
+ await this.peopleService.findById(memberId, organizationId);
+ const attachments = await this.attachmentsService.getAttachments(
+ organizationId,
+ memberId,
</file context>
Suggested change
| const attachments = await this.attachmentsService.getAttachments( | |
| organizationId, | |
| memberId, | |
| entityType, | |
| ); | |
| const attachments = await this.attachmentsService.getAttachmentMetadata( | |
| organizationId, | |
| memberId, | |
| entityType, | |
| ); |
Contributor
There was a problem hiding this comment.
Won't fix — the getAttachments call generates signed URLs as a side effect, but the overhead is minimal (only runs on delete, which is rare). Adding an ID-only query path would require a new method on AttachmentsService for marginal benefit.
Contributor
There was a problem hiding this comment.
Understood—thanks for the context.
5 tasks
* fix(people): address fourth round of cubic review findings - add findFirst mock to test spec for undoVendorRevocation - prefix checklist evidence filenames with ID to prevent ZIP collisions - wrap sync call in try/catch so revocation stands on sync failure - prevent non-expandable checklist rows from toggling open - normalize preset date ranges to start of day - hide pending invitations when date filters are active - use controlled tab value in ToDoOverview for async data - revalidate from server on error instead of restoring stale snapshot Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(people): log sync failures instead of swallowing silently Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
@cubic-dev-ai re-review this |
Contributor
@Marfuen I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
7 issues found across 82 files
Confidence score: 2/5
- There is a high-impact multi-tenant integrity risk in
packages/db/prisma/schema/offboarding-checklist.prisma: missing DB-level tenant scoping on related foreign keys can permit cross-organization references. apps/api/src/offboarding-checklist/access-revocation.service.tshas two medium-high issues where sync failures inrevokeAllVendorAccess/undoVendorRevocationcan return errors after DB state already changed, creating confusing partial-success behavior for users.apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/AccessRevocationList.tsxuses a singleprocessingVendorId, which can produce racey row-action UI state when requests overlap; there is also a performance concern from N+1 attachment loading inapps/api/src/offboarding-checklist/offboarding-checklist.service.ts.- Pay close attention to
packages/db/prisma/schema/offboarding-checklist.prisma,apps/api/src/offboarding-checklist/access-revocation.service.ts, andapps/app/src/app/(app)/[orgId]/people/[employeeId]/components/AccessRevocationList.tsx- tenant isolation, consistency after sync errors, and concurrent action handling are the key merge risks.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/AccessRevocationList.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/AccessRevocationList.tsx:52">
P2: Using a single `processingVendorId` causes incorrect loading/disabled behavior when multiple row actions overlap. Track processing per vendor (or globally lock actions) to avoid prematurely re-enabling controls during in-flight requests.</violation>
</file>
<file name="apps/api/src/offboarding-checklist/offboarding-checklist.service.ts">
<violation number="1" location="apps/api/src/offboarding-checklist/offboarding-checklist.service.ts:49">
P2: `createTemplateItem` has a race condition for `sortOrder`; concurrent creates can assign the same order value.</violation>
</file>
<file name="apps/app/src/hooks/use-access-revocations.ts">
<violation number="1" location="apps/app/src/hooks/use-access-revocations.ts:49">
P2: Add a file-size check before base64 conversion to avoid loading oversized evidence files into browser memory.</violation>
</file>
<file name="apps/app/src/hooks/use-employment-evidence.ts">
<violation number="1" location="apps/app/src/hooks/use-employment-evidence.ts:32">
P2: Add a client-side file size check before `readAsDataURL` to avoid loading oversized files into memory before the server rejects them.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/components/FrameworkControls.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/components/FrameworkControls.tsx:177">
P2: This per-row computation repeatedly scans tasks and re-sorts evidence submissions, causing avoidable O(n×m) render-time work for larger control lists.</violation>
</file>
<file name="apps/api/src/cloud-security/ai-remediation.prompt.ts">
<violation number="1" location="apps/api/src/cloud-security/ai-remediation.prompt.ts:182">
P2: The prompt mixes IAM action syntax with SDK command-class syntax (`iam:CreateServiceLinkedRoleCommand`), which can make generated fix steps use an invalid command name.</violation>
<violation number="2" location="apps/api/src/cloud-security/ai-remediation.prompt.ts:182">
P2: Use the AWS SDK command class name without the `iam:` prefix to avoid generating invalid `command` values.</violation>
</file>
<file name="apps/api/src/cloud-security/plan-normalizer.ts">
<violation number="1" location="apps/api/src/cloud-security/plan-normalizer.ts:59">
P2: Whitespace-only `AWSServiceName` values are treated as valid, so malformed SLR steps can bypass normalization and still fail at execution time.</violation>
</file>
<file name="apps/api/src/cloud-security/aws-command-executor.ts">
<violation number="1" location="apps/api/src/cloud-security/aws-command-executor.ts:411">
P2: Trailing space in `'invalidparameter '` will miss error messages where "InvalidParameter" is followed by punctuation or appears at the end of the string. Since `'invalidparametervalue'` is already matched by an earlier check (short-circuits via `||`), you can safely use `'invalidparameter'` without the space—or add a word-boundary-style check.</violation>
<violation number="2" location="apps/api/src/cloud-security/aws-command-executor.ts:740">
P2: Repair equality check only compares `params`, ignoring potential `service`/`command` changes from the AI callback. If the repair function fixes the command name (e.g., correcting `PutBucketPolicyCommand` → `PutBucketEncryptionCommand`) but keeps similar params, the fix is silently discarded.
Compare the full step instead of just params.</violation>
</file>
<file name="apps/app/src/hooks/use-offboarding-checklist.ts">
<violation number="1" location="apps/app/src/hooks/use-offboarding-checklist.ts:63">
P2: Add a file-size limit check before base64 conversion to avoid high client memory usage on large evidence uploads.</violation>
</file>
<file name="apps/api/src/people/people.controller.ts">
<violation number="1" location="apps/api/src/people/people.controller.ts:627">
P2: This delete path calls `getAttachments`, which unnecessarily generates signed URLs for every attachment even though only IDs are used for existence checking.</violation>
</file>
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
4 tasks
- guard sync errors in revokeAllVendorAccess and undoVendorRevocation - remove unused TodosOverview component (dead code) - batch attachment queries in getMemberChecklist (N+1 → 1 query) - extract fileToBase64 into shared lib/file-utils.ts Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
@cubic-dev-ai re-review this |
Contributor
@Marfuen I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
14 issues found across 82 files
Confidence score: 2/5
- High-confidence, high-severity test breakages are likely in
apps/api/src/offboarding-checklist/offboarding-checklist.service.spec.ts(missingmockDb.$transaction,attachment,member, andgetPresignedDownloadUrlmocks), which makes CI failures very likely until fixed. - There are concrete user-impact risks in export/data paths:
apps/api/src/offboarding-checklist/offboarding-export.service.tscan swallow archiver errors (possible incomplete ZIP reported as success), andapps/api/src/tasks/evidence-export/evidence-data-loader.tsmay still cause memory spikes due to unbounded header loading. - A few medium-severity correctness gaps remain (race handling in
apps/api/src/offboarding-checklist/access-revocation.service.ts, partial-evidence validation inapps/api/src/offboarding-checklist/dto/complete-checklist-item.dto.ts, and prompt command-format inconsistency inapps/api/src/cloud-security/ai-remediation.prompt.ts), so this doesn’t look like a safe merge yet. - Pay close attention to
apps/api/src/offboarding-checklist/offboarding-checklist.service.spec.ts,apps/api/src/offboarding-checklist/offboarding-export.service.ts, andapps/api/src/tasks/evidence-export/evidence-data-loader.ts- likely test failures and export reliability/performance regressions need resolution before merge.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/cloud-security/ai-remediation.prompt.ts">
<violation number="1" location="apps/api/src/cloud-security/ai-remediation.prompt.ts:182">
P2: The added instruction mixes IAM action syntax into the command name (`iam:CreateServiceLinkedRoleCommand`), which conflicts with the prompt’s required SDK class-name format and can produce invalid command output.</violation>
</file>
<file name="apps/api/src/offboarding-checklist/dto/complete-checklist-item.dto.ts">
<violation number="1" location="apps/api/src/offboarding-checklist/dto/complete-checklist-item.dto.ts:14">
P2: Evidence fields are validated independently, so partial evidence payloads can pass and then be silently ignored. Enforce an all-or-none constraint for `fileName`, `fileType`, and `fileData`.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/OffboardingSummaryCard.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/OffboardingSummaryCard.tsx:22">
P2: `Days since` can render negative values when `offboardDate` is in the future. Clamp or otherwise handle future dates to avoid incorrect elapsed-day output.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/components/FrameworkControls.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/components/FrameworkControls.tsx:33">
P2: This component substantially duplicates the existing requirement controls table logic; extract shared table logic/components to avoid maintenance drift.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/OffboardingChecklistItem.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/OffboardingChecklistItem.tsx:377">
P2: The evidence dropzone is not disabled while `isProcessing` is true, so users can still click/drop files that are then silently ignored by the upload guard.</violation>
</file>
<file name="apps/api/src/tasks/evidence-export/evidence-data-loader.ts">
<violation number="1" location="apps/api/src/tasks/evidence-export/evidence-data-loader.ts:130">
P1: `getAutomationHeaders` fetches all run headers in one shot, which can still cause large memory spikes for high-volume tasks. Use paginated/grouped queries instead of unbounded `findMany` here.</violation>
</file>
<file name="apps/api/src/offboarding-checklist/offboarding-export.service.ts">
<violation number="1" location="apps/api/src/offboarding-checklist/offboarding-export.service.ts:34">
P1: Archiver errors are swallowed instead of being propagated, which can return incomplete ZIP exports as apparent success.</violation>
<violation number="2" location="apps/api/src/offboarding-checklist/offboarding-export.service.ts:221">
P2: Apply CSV formula-injection prefixing before quote-escaping in `escapeCsvField` to follow the repository’s required sanitization order.
(Based on your team's feedback about CSV formula-injection protection ordering.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/api/src/tasks/evidence-export/evidence-export.service.spec.ts">
<violation number="1" location="apps/api/src/tasks/evidence-export/evidence-export.service.spec.ts:558">
P2: Test does not verify absence of the bulk `integrationCheckRun.findMany` call (without `checkId`). The test name says "instead of all runs at once" but only checks that per-automation calls happen; a regression that adds back the bulk load while keeping per-automation calls would still pass, silently reintroducing the OOM.</violation>
</file>
<file name="apps/api/src/offboarding-checklist/access-revocation.service.ts">
<violation number="1" location="apps/api/src/offboarding-checklist/access-revocation.service.ts:38">
P2: Avoid per-vendor attachment fetches in the loop; this introduces N+1 DB/signing work and slows large revocation lists.</violation>
<violation number="2" location="apps/api/src/offboarding-checklist/access-revocation.service.ts:105">
P2: Handle unique-constraint races when creating revocations so duplicate requests return a 400 instead of a server error.</violation>
</file>
<file name="apps/api/src/offboarding-checklist/offboarding-checklist.service.spec.ts">
<violation number="1" location="apps/api/src/offboarding-checklist/offboarding-checklist.service.spec.ts:3">
P0: Missing `$transaction` mock in `mockDb`: `seedDefaultsIfNeeded()` calls `db.$transaction(...)` but `mockDb.$transaction` is undefined. The `getTemplate` seeding test will fail.</violation>
<violation number="2" location="apps/api/src/offboarding-checklist/offboarding-checklist.service.spec.ts:3">
P0: Missing `member` mock in `mockDb`: `AccessRevocationService.revokeVendorAccess()` calls `db.member.findFirst(...)` but `mockDb` does not define a `member` property. All three `revokeVendorAccess` tests will throw a TypeError.</violation>
<violation number="3" location="apps/api/src/offboarding-checklist/offboarding-checklist.service.spec.ts:149">
P0: Missing `attachment` mock in `mockDb` and missing `getPresignedDownloadUrl` mock: `getMemberChecklist()` uses `db.attachment.findMany(...)` and `attachmentsService.getPresignedDownloadUrl(...)` but neither is mocked. The `getAttachments` mock on line 424 is dead code since the service now uses `db.attachment.findMany` directly.</violation>
</file>
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| @@ -0,0 +1,545 @@ | |||
| import { BadRequestException, NotFoundException } from '@nestjs/common'; | |||
|
|
|||
| const mockDb = { | |||
Contributor
There was a problem hiding this comment.
P0: Missing $transaction mock in mockDb: seedDefaultsIfNeeded() calls db.$transaction(...) but mockDb.$transaction is undefined. The getTemplate seeding test will fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/offboarding-checklist/offboarding-checklist.service.spec.ts, line 3:
<comment>Missing `$transaction` mock in `mockDb`: `seedDefaultsIfNeeded()` calls `db.$transaction(...)` but `mockDb.$transaction` is undefined. The `getTemplate` seeding test will fail.</comment>
<file context>
@@ -0,0 +1,545 @@
+import { BadRequestException, NotFoundException } from '@nestjs/common';
+
+const mockDb = {
+ offboardingChecklistTemplate: {
+ findMany: jest.fn(),
</file context>
| completedBy: { id: 'usr_1', name: 'Test User' }, | ||
| }, | ||
| ]); | ||
| mockAttachmentsService.getAttachments.mockResolvedValue([ |
Contributor
There was a problem hiding this comment.
P0: Missing attachment mock in mockDb and missing getPresignedDownloadUrl mock: getMemberChecklist() uses db.attachment.findMany(...) and attachmentsService.getPresignedDownloadUrl(...) but neither is mocked. The getAttachments mock on line 424 is dead code since the service now uses db.attachment.findMany directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/offboarding-checklist/offboarding-checklist.service.spec.ts, line 149:
<comment>Missing `attachment` mock in `mockDb` and missing `getPresignedDownloadUrl` mock: `getMemberChecklist()` uses `db.attachment.findMany(...)` and `attachmentsService.getPresignedDownloadUrl(...)` but neither is mocked. The `getAttachments` mock on line 424 is dead code since the service now uses `db.attachment.findMany` directly.</comment>
<file context>
@@ -0,0 +1,545 @@
+ completedBy: { id: 'usr_1', name: 'Test User' },
+ },
+ ]);
+ mockAttachmentsService.getAttachments.mockResolvedValue([
+ { id: 'att_1', name: 'evidence.pdf' },
+ ]);
</file context>
| @@ -0,0 +1,545 @@ | |||
| import { BadRequestException, NotFoundException } from '@nestjs/common'; | |||
|
|
|||
| const mockDb = { | |||
Contributor
There was a problem hiding this comment.
P0: Missing member mock in mockDb: AccessRevocationService.revokeVendorAccess() calls db.member.findFirst(...) but mockDb does not define a member property. All three revokeVendorAccess tests will throw a TypeError.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/offboarding-checklist/offboarding-checklist.service.spec.ts, line 3:
<comment>Missing `member` mock in `mockDb`: `AccessRevocationService.revokeVendorAccess()` calls `db.member.findFirst(...)` but `mockDb` does not define a `member` property. All three `revokeVendorAccess` tests will throw a TypeError.</comment>
<file context>
@@ -0,0 +1,545 @@
+import { BadRequestException, NotFoundException } from '@nestjs/common';
+
+const mockDb = {
+ offboardingChecklistTemplate: {
+ findMany: jest.fn(),
</file context>
| } | ||
|
|
||
| const [appRuns, customRuns] = await Promise.all([ | ||
| db.integrationCheckRun.findMany({ |
Contributor
There was a problem hiding this comment.
P1: getAutomationHeaders fetches all run headers in one shot, which can still cause large memory spikes for high-volume tasks. Use paginated/grouped queries instead of unbounded findMany here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/tasks/evidence-export/evidence-data-loader.ts, line 130:
<comment>`getAutomationHeaders` fetches all run headers in one shot, which can still cause large memory spikes for high-volume tasks. Use paginated/grouped queries instead of unbounded `findMany` here.</comment>
<file context>
@@ -0,0 +1,385 @@
+ }
+
+ const [appRuns, customRuns] = await Promise.all([
+ db.integrationCheckRun.findMany({
+ where: { taskId },
+ select: {
</file context>
| output: NodeJS.WritableStream; | ||
| }) { | ||
| const archive = archiver('zip', { zlib: { level: 9 } }); | ||
| archive.on('error', () => { archive.abort(); }); |
Contributor
There was a problem hiding this comment.
P1: Archiver errors are swallowed instead of being propagated, which can return incomplete ZIP exports as apparent success.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/offboarding-checklist/offboarding-export.service.ts, line 34:
<comment>Archiver errors are swallowed instead of being propagated, which can return incomplete ZIP exports as apparent success.</comment>
<file context>
@@ -0,0 +1,226 @@
+ output: NodeJS.WritableStream;
+ }) {
+ const archive = archiver('zip', { zlib: { level: 9 } });
+ archive.on('error', () => { archive.abort(); });
+ archive.pipe(output);
+
</file context>
| onDrop={onFileDrop} | ||
| onDragOver={(e) => e.preventDefault()} | ||
| onClick={() => dropzoneInputRef.current?.click()} | ||
| className="flex cursor-pointer flex-col items-center gap-2 rounded-md border-2 border-dashed border-muted-foreground/25 px-4 py-6 text-center transition hover:border-muted-foreground/50 hover:bg-muted/25" |
Contributor
There was a problem hiding this comment.
P2: The evidence dropzone is not disabled while isProcessing is true, so users can still click/drop files that are then silently ignored by the upload guard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/OffboardingChecklistItem.tsx, line 377:
<comment>The evidence dropzone is not disabled while `isProcessing` is true, so users can still click/drop files that are then silently ignored by the upload guard.</comment>
<file context>
@@ -0,0 +1,412 @@
+ onDrop={onFileDrop}
+ onDragOver={(e) => e.preventDefault()}
+ onClick={() => dropzoneInputRef.current?.click()}
+ className="flex cursor-pointer flex-col items-center gap-2 rounded-md border-2 border-dashed border-muted-foreground/25 px-4 py-6 text-center transition hover:border-muted-foreground/50 hover:bg-muted/25"
+ >
+ <Upload size={20} className="text-muted-foreground" />
</file context>
| } | ||
|
|
||
| function escapeCsvField(value: string): string { | ||
| const escaped = value.replace(/"/g, '""'); |
Contributor
There was a problem hiding this comment.
P2: Apply CSV formula-injection prefixing before quote-escaping in escapeCsvField to follow the repository’s required sanitization order.
(Based on your team's feedback about CSV formula-injection protection ordering.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/offboarding-checklist/offboarding-export.service.ts, line 221:
<comment>Apply CSV formula-injection prefixing before quote-escaping in `escapeCsvField` to follow the repository’s required sanitization order.
(Based on your team's feedback about CSV formula-injection protection ordering.) </comment>
<file context>
@@ -0,0 +1,226 @@
+}
+
+function escapeCsvField(value: string): string {
+ const escaped = value.replace(/"/g, '""');
+ if (/^[=+\-@\t\r]/.test(escaped)) {
+ return `'${escaped}`;
</file context>
| // Verify per-automation loading: findMany is called with individual | ||
| // checkId filters (not a single bulk load of all results). | ||
| const findManyCalls = mockDb.integrationCheckRun.findMany.mock.calls; | ||
| const perAutomationCalls = findManyCalls.filter( |
Contributor
There was a problem hiding this comment.
P2: Test does not verify absence of the bulk integrationCheckRun.findMany call (without checkId). The test name says "instead of all runs at once" but only checks that per-automation calls happen; a regression that adds back the bulk load while keeping per-automation calls would still pass, silently reintroducing the OOM.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/tasks/evidence-export/evidence-export.service.spec.ts, line 558:
<comment>Test does not verify absence of the bulk `integrationCheckRun.findMany` call (without `checkId`). The test name says "instead of all runs at once" but only checks that per-automation calls happen; a regression that adds back the bulk load while keeping per-automation calls would still pass, silently reintroducing the OOM.</comment>
<file context>
@@ -473,6 +473,101 @@ describe('EvidenceExportService — streaming ZIPs', () => {
+ // Verify per-automation loading: findMany is called with individual
+ // checkId filters (not a single bulk load of all results).
+ const findManyCalls = mockDb.integrationCheckRun.findMany.mock.calls;
+ const perAutomationCalls = findManyCalls.filter(
+ (call: unknown[]) =>
+ (call[0] as { where: { checkId?: string } }).where.checkId,
</file context>
| const revocation = revocationMap.get(vendor.id); | ||
| const domain = vendor.website?.replace(/^https?:\/\//, '').replace(/\/.*$/, '') ?? null; | ||
| const evidence = revocation | ||
| ? await this.attachmentsService.getAttachments( |
Contributor
There was a problem hiding this comment.
P2: Avoid per-vendor attachment fetches in the loop; this introduces N+1 DB/signing work and slows large revocation lists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/offboarding-checklist/access-revocation.service.ts, line 38:
<comment>Avoid per-vendor attachment fetches in the loop; this introduces N+1 DB/signing work and slows large revocation lists.</comment>
<file context>
@@ -0,0 +1,279 @@
+ const revocation = revocationMap.get(vendor.id);
+ const domain = vendor.website?.replace(/^https?:\/\//, '').replace(/\/.*$/, '') ?? null;
+ const evidence = revocation
+ ? await this.attachmentsService.getAttachments(
+ organizationId,
+ revocation.id,
</file context>
| ); | ||
| } | ||
|
|
||
| const revocation = await db.offboardingAccessRevocation.create({ |
Contributor
There was a problem hiding this comment.
P2: Handle unique-constraint races when creating revocations so duplicate requests return a 400 instead of a server error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/offboarding-checklist/access-revocation.service.ts, line 105:
<comment>Handle unique-constraint races when creating revocations so duplicate requests return a 400 instead of a server error.</comment>
<file context>
@@ -0,0 +1,279 @@
+ );
+ }
+
+ const revocation = await db.offboardingAccessRevocation.create({
+ data: {
+ organizationId,
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automated pull request to release the candidate branch into production, which will trigger a deployment.
It was created by the [Production PR] action.
Summary by cubic
Ships the offboarding checklist with vendor access‑revocation tracking, evidence upload/export, and employment event dates/evidence across People and integrations. Also improves Cloud Security auto‑remediation (required‑param validation, deterministic SLR normalization, universal step‑repair) and enables API key/service token mutations with clear audit attribution; sets Frameworks to default to a Controls view; and fixes evidence export OOMs with streaming.
New Features
@trycompai/design-system→1.1.19.Bug Fixes
apps/app/src/lib/file-utils.ts; remove unused TodosOverview component.Written for commit dbc364c. Summary will update on new commits. Review in cubic