Conversation
feat(people): add employment events tracking and offboarding checklist
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
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) => { |
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>
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.
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
…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>
* 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>
|
@cubic-dev-ai re-review this |
@Marfuen I have started the AI code review. It will take a few minutes to complete. |
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
- 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>
|
@cubic-dev-ai re-review this |
@Marfuen I have started the AI code review. It will take a few minutes to complete. |
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/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:33">
P2: This component substantially duplicates the existing requirement controls table logic; extract shared table logic/components to avoid maintenance drift.</violation>
<violation number="2" 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>
<violation number="3" 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/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>
<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>
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
| // 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( |
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>
- propagate archiver errors to output stream to prevent truncated ZIPs - clamp "days since" to 0 for future offboard dates - disable evidence dropzone while upload is processing - batch vendor attachment queries in getAccessRevocations (N+1 fix) - catch P2002 race on revocation create, return 400 instead of 500 Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- add member, attachment, $transaction mocks to mockDb - add getPresignedDownloadUrl mock to attachmentsService - mock db.attachment.findMany for batched checklist/revocation queries - add member.findFirst mock to revokeVendorAccess tests Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@cubic-dev-ai re-review this |
@Marfuen I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
10 issues found across 82 files
Confidence score: 2/5
- There are a couple of high-confidence, high-impact risks (sev 7/10) that make this merge risky right now, especially around data integrity and failure recovery.
- In
packages/db/prisma/schema/offboarding-checklist.prisma, cascading FK deletes can remove revocation/offboarding history, which creates concrete data-loss risk for historical records. - In
apps/api/src/cloud-security/aws-command-executor.ts, exceptions thrown byrepairStepcan bypass the auto-rollback path, leaving partially completed remediation steps unreverted. - Pay close attention to
packages/db/prisma/schema/offboarding-checklist.prisma,apps/api/src/cloud-security/aws-command-executor.ts, andapps/api/src/tasks/evidence-export/evidence-data-loader.ts- they carry the highest user-impacting risks (history loss, rollback gaps, and pagination 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]/frameworks/[frameworkInstanceId]/components/FrameworkControls.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/components/FrameworkControls.tsx:177">
P2: This row-level rendering does repeated O(n) task scans and evidence sorting per control, which can noticeably degrade table performance for large datasets. Precompute per-control metrics once (e.g., in `useMemo`) and read them in the row render.</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:146">
P2: Header pagination is not deterministic: sorting only by `createdAt` while paging by `id` cursor can skip or duplicate runs when timestamps tie.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/OffboardingChecklist.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/people/[employeeId]/components/OffboardingChecklist.tsx:21">
P2: Handle hook errors explicitly; otherwise failed checklist fetches are shown as an empty checklist configuration state.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/people/all/components/TeamMembersClient.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/people/all/components/TeamMembersClient.tsx:641">
P2: Validate or normalize date order before applying the range; selecting an end date before the start date currently results in an invalid filter that can hide all rows.</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:246">
P2: Prevent repeated toggles while update requests are in flight; otherwise overlapping PATCH calls can race and save the wrong final enabled state.</violation>
</file>
<file name="packages/db/prisma/schema/offboarding-checklist.prisma">
<violation number="1" location="packages/db/prisma/schema/offboarding-checklist.prisma:31">
P2: Cascading member deletes will remove offboarding checklist history; use a non-cascading FK behavior for this history table.</violation>
<violation number="2" location="packages/db/prisma/schema/offboarding-checklist.prisma:50">
P1: Revocation history is vulnerable to data loss because member/vendor FKs use cascading deletes.</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:267">
P2: `confirmed` reports attempted inserts, not actual created rows, when `createMany(..., skipDuplicates: true)` skips duplicates.</violation>
</file>
<file name="apps/api/src/people/people.controller.ts">
<violation number="1" location="apps/api/src/people/people.controller.ts:627">
P2: Use metadata lookup instead of full attachment URL generation in delete flow to avoid unnecessary per-attachment signed URL work.</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:733">
P1: Unhandled exception from `repairStep` callback bypasses auto-rollback. If the repair callback throws (LLM timeout, network error, etc.), the error escapes the `catch` block and skips the auto-rollback of already-completed steps. Wrap the `params.repairStep(...)` call in a try-catch so failures fall through to the normal error-handling path.</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
| notes String? | ||
|
|
||
| organization Organization @relation(fields: [organizationId], references: [id], onDelete: Cascade) | ||
| member Member @relation(fields: [memberId], references: [id], onDelete: Cascade) |
There was a problem hiding this comment.
P1: Revocation history is vulnerable to data loss because member/vendor FKs use cascading deletes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/prisma/schema/offboarding-checklist.prisma, line 50:
<comment>Revocation history is vulnerable to data loss because member/vendor FKs use cascading deletes.</comment>
<file context>
@@ -0,0 +1,57 @@
+ notes String?
+
+ organization Organization @relation(fields: [organizationId], references: [id], onDelete: Cascade)
+ member Member @relation(fields: [memberId], references: [id], onDelete: Cascade)
+ vendor Vendor @relation(fields: [vendorId], references: [id], onDelete: Cascade)
+ revokedBy User? @relation("AccessRevocationRevokedBy", fields: [revokedById], references: [id], onDelete: SetNull)
</file context>
There was a problem hiding this comment.
Won't fix — cascade delete on Member and Vendor FKs is intentional. When a member is fully removed from the org, their revocation records have no purpose. Same for vendors. This is the standard pattern across the codebase (risks, controls, tasks all cascade from their parent entities). We already changed the User FK to SetNull to preserve audit attribution.
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
| const refined = await params.repairStep({ | ||
| step: originalStep, | ||
| awsError: message, | ||
| stepIndex: i, | ||
| }); |
There was a problem hiding this comment.
P1: Unhandled exception from repairStep callback bypasses auto-rollback. If the repair callback throws (LLM timeout, network error, etc.), the error escapes the catch block and skips the auto-rollback of already-completed steps. Wrap the params.repairStep(...) call in a try-catch so failures fall through to the normal error-handling path.
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 733:
<comment>Unhandled exception from `repairStep` callback bypasses auto-rollback. If the repair callback throws (LLM timeout, network error, etc.), the error escapes the `catch` block and skips the auto-rollback of already-completed steps. Wrap the `params.repairStep(...)` call in a try-catch so failures fall through to the normal error-handling path.</comment>
<file context>
@@ -607,22 +687,91 @@ export async function executePlanSteps(params: {
+ `Step ${i + 1} (${originalStep.service}:${originalStep.command}) ` +
+ `failed with validation error — attempting AI step repair`,
+ );
+ const refined = await params.repairStep({
+ step: originalStep,
+ awsError: message,
</file context>
| const refined = await params.repairStep({ | |
| step: originalStep, | |
| awsError: message, | |
| stepIndex: i, | |
| }); | |
| let refined: AwsCommandStep | null = null; | |
| try { | |
| refined = await params.repairStep({ | |
| step: originalStep, | |
| awsError: message, | |
| stepIndex: i, | |
| }); | |
| } catch (repairErr) { | |
| console.error( | |
| `AI step repair callback failed for ${originalStep.command}:`, | |
| repairErr instanceof Error ? repairErr.message : repairErr, | |
| ); | |
| } |
| paginatedItems.map(({ control, requirements }) => { | ||
| const policies = control.policies ?? []; | ||
| const documentTypes = control.controlDocumentTypes ?? []; | ||
| const counts = getRequirementArtifactCounts([control], tasks, evidenceSubmissions); |
There was a problem hiding this comment.
P2: This row-level rendering does repeated O(n) task scans and evidence sorting per control, which can noticeably degrade table performance for large datasets. Precompute per-control metrics once (e.g., in useMemo) and read them in the row render.
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]/frameworks/[frameworkInstanceId]/components/FrameworkControls.tsx, line 177:
<comment>This row-level rendering does repeated O(n) task scans and evidence sorting per control, which can noticeably degrade table performance for large datasets. Precompute per-control metrics once (e.g., in `useMemo`) and read them in the row render.</comment>
<file context>
@@ -0,0 +1,289 @@
+ paginatedItems.map(({ control, requirements }) => {
+ const policies = control.policies ?? [];
+ const documentTypes = control.controlDocumentTypes ?? [];
+ const counts = getRequirementArtifactCounts([control], tasks, evidenceSubmissions);
+ const status = getControlStatus(
+ policies,
</file context>
| @@ -0,0 +1,405 @@ | |||
| import { NotFoundException } from '@nestjs/common'; | |||
There was a problem hiding this comment.
P2: Header pagination is not deterministic: sorting only by createdAt while paging by id cursor can skip or duplicate runs when timestamps tie.
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 146:
<comment>Header pagination is not deterministic: sorting only by `createdAt` while paging by `id` cursor can skip or duplicate runs when timestamps tie.</comment>
<file context>
@@ -0,0 +1,405 @@
+ select: { provider: { select: { slug: true, name: true } } },
+ },
+ },
+ orderBy: { createdAt: 'desc' },
+ take: HEADER_BATCH,
+ ...(appCursor ? { cursor: { id: appCursor }, skip: 1 } : {}),
</file context>
| canEdit, | ||
| offboardDate, | ||
| }: OffboardingChecklistProps) { | ||
| const { |
There was a problem hiding this comment.
P2: Handle hook errors explicitly; otherwise failed checklist fetches are shown as an empty checklist configuration state.
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/OffboardingChecklist.tsx, line 21:
<comment>Handle hook errors explicitly; otherwise failed checklist fetches are shown as an empty checklist configuration state.</comment>
<file context>
@@ -0,0 +1,168 @@
+ canEdit,
+ offboardDate,
+}: OffboardingChecklistProps) {
+ const {
+ checklist,
+ isLoading,
</file context>
| }; | ||
|
|
||
| const handleApply = () => { | ||
| onApply(draftFrom, draftTo); |
There was a problem hiding this comment.
P2: Validate or normalize date order before applying the range; selecting an end date before the start date currently results in an invalid filter that can hide all rows.
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/all/components/TeamMembersClient.tsx, line 641:
<comment>Validate or normalize date order before applying the range; selecting an end date before the start date currently results in an invalid filter that can hide all rows.</comment>
<file context>
@@ -503,3 +569,178 @@ export function TeamMembersClient({
+ };
+
+ const handleApply = () => {
+ onApply(draftFrom, draftTo);
+ setOpen(false);
+ };
</file context>
| onApply(draftFrom, draftTo); | |
| if (draftFrom && draftTo && draftFrom > draftTo) { | |
| onApply(draftTo, draftFrom); | |
| } else { | |
| onApply(draftFrom, draftTo); | |
| } |
| <HStack gap="sm"> | ||
| <Switch | ||
| checked={item.isEnabled} | ||
| disabled={!canUpdate} |
There was a problem hiding this comment.
P2: Prevent repeated toggles while update requests are in flight; otherwise overlapping PATCH calls can race and save the wrong final enabled state.
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/settings/components/OffboardingChecklistSettings.tsx, line 246:
<comment>Prevent repeated toggles while update requests are in flight; otherwise overlapping PATCH calls can race and save the wrong final enabled state.</comment>
<file context>
@@ -0,0 +1,353 @@
+ <HStack gap="sm">
+ <Switch
+ checked={item.isEnabled}
+ disabled={!canUpdate}
+ onCheckedChange={(next) =>
+ onToggleEnabled({ item, next: Boolean(next) })
</file context>
| notes String? | ||
|
|
||
| organization Organization @relation(fields: [organizationId], references: [id], onDelete: Cascade) | ||
| member Member @relation(fields: [memberId], references: [id], onDelete: Cascade) |
There was a problem hiding this comment.
P2: Cascading member deletes will remove offboarding checklist history; use a non-cascading FK behavior for this history table.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/prisma/schema/offboarding-checklist.prisma, line 31:
<comment>Cascading member deletes will remove offboarding checklist history; use a non-cascading FK behavior for this history table.</comment>
<file context>
@@ -0,0 +1,57 @@
+ notes String?
+
+ organization Organization @relation(fields: [organizationId], references: [id], onDelete: Cascade)
+ member Member @relation(fields: [memberId], references: [id], onDelete: Cascade)
+ templateItem OffboardingChecklistTemplate? @relation(fields: [templateItemId], references: [id], onDelete: SetNull)
+ completedBy User? @relation("OffboardingChecklistCompletedBy", fields: [completedById], references: [id], onDelete: SetNull)
</file context>
| this.logger.warn(`Failed to sync access revocation completion for member ${memberId}`, err); | ||
| } | ||
|
|
||
| return { confirmed: toCreate.length }; |
There was a problem hiding this comment.
P2: confirmed reports attempted inserts, not actual created rows, when createMany(..., skipDuplicates: true) skips duplicates.
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 267:
<comment>`confirmed` reports attempted inserts, not actual created rows, when `createMany(..., skipDuplicates: true)` skips duplicates.</comment>
<file context>
@@ -0,0 +1,312 @@
+ this.logger.warn(`Failed to sync access revocation completion for member ${memberId}`, err);
+ }
+
+ return { confirmed: toCreate.length };
+ }
+
</file context>
| ) { | ||
| const entityType = this.resolveEventType(eventType); | ||
| await this.peopleService.findById(memberId, organizationId); | ||
| const attachments = await this.attachmentsService.getAttachments( |
There was a problem hiding this comment.
P2: Use metadata lookup instead of full attachment URL generation in delete flow to avoid unnecessary per-attachment signed URL work.
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>Use metadata lookup instead of full attachment URL generation in delete flow to avoid unnecessary per-attachment signed URL work.</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>
|
🎉 This PR is included in version 3.60.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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; handle P2002 race on revocation create with a 400.$transaction,getPresignedDownloadUrl,attachment.findMany) for offboarding specs and revokeVendorAccess tests to stabilize CI.Written for commit b8bf01d. Summary will update on new commits. Review in cubic