[dev] [carhartlewis] lewis/comp-framework-controls#2593
Conversation
…points - Implemented new API endpoints for creating custom frameworks and requirements in the FrameworksController. - Added DTOs for CreateCustomFrameworkDto, CreateCustomRequirementDto, LinkRequirementsDto, and LinkControlsDto. - Enhanced findAvailable method to filter frameworks by organization ID. - Introduced client components for adding and linking requirements, including AddCustomRequirementSheet and LinkRequirementSheet. - Updated FrameworkOverview to include new actions for managing custom requirements. - Added UI components for creating custom frameworks and linking existing controls. This update enhances the framework management capabilities, allowing users to create and manage custom frameworks and their associated requirements effectively.
|
Lewis Carhart seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
There was a problem hiding this comment.
4 issues found across 25 files
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/frameworks/frameworks.service.ts">
<violation number="1" location="apps/api/src/frameworks/frameworks.service.ts:174">
P1: Multi-tenancy gap: the requirement lookup doesn't scope by `organizationId`, so on a shared global framework, one org could link controls to another org's custom requirement. Add an `organizationId` filter matching the pattern used in `linkRequirements`.</violation>
<violation number="2" location="apps/api/src/frameworks/frameworks.service.ts:260">
P2: Calling `linkRequirements` multiple times with the same IDs will create duplicate requirement records each time, since there's no unique constraint and no deduplication check. Consider checking for existing requirements with the same `identifier` and `frameworkId` before inserting, or adding a uniqueness constraint.</violation>
</file>
<file name="apps/api/src/frameworks/dto/create-custom-requirement.dto.ts">
<violation number="1" location="apps/api/src/frameworks/dto/create-custom-requirement.dto.ts:13">
P2: `identifier` accepts an empty string. Add `@MinLength(1)` to match the validation on `name` — an empty identifier is almost certainly invalid.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/components/LinkRequirementSheet.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/components/LinkRequirementSheet.tsx:49">
P2: The SWR loading state is not handled. When the sheet opens and data is fetching, `options` defaults to `[]`, so users briefly see "No requirements available" before the real data arrives. Destructure `isLoading` from `useSWR` and show a loading indicator instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code reviewScoping Architectural note (not blocking): The rest of this codebase treats Found 2 issues:
comp/apps/api/src/frameworks/frameworks.service.ts Lines 136 to 145 in c3a50db comp/apps/api/src/frameworks/frameworks.service.ts Lines 384 to 391 in c3a50db
comp/apps/api/src/frameworks/frameworks.service.ts Lines 258 to 270 in c3a50db 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…ts to controls - Implemented new API endpoints in ControlsController for linking existing policies, tasks, and requirements to a control. - Added corresponding DTOs: LinkPoliciesDto, LinkTasksDto, and LinkRequirementsToControlDto. - Enhanced ControlsService with methods to handle linking logic and validation. - Updated UI components to support linking actions, including LinkPolicySheet, LinkTaskSheet, and LinkRequirementForControlSheet. - Improved data fetching with useControlOptions hook for better management of linked items. This update enhances the control management capabilities, allowing users to effectively link existing resources to controls.
There was a problem hiding this comment.
2 issues found across 19 files (changes from recent commits).
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]/requirements/[requirementKey]/page.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/requirements/[requirementKey]/page.tsx:32">
P2: Remove this debug `console.log` block from the server-rendered page; it logs internal API error payloads on every request and can leak unnecessary details to production logs.</violation>
</file>
<file name="apps/api/src/controls/controls.service.ts">
<violation number="1" location="apps/api/src/controls/controls.service.ts:297">
P2: `linkRequirements` can return an incorrect `count` because it reports `validMappings.length` even when `createMany(..., skipDuplicates: true)` skips duplicates.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
…tables Previously the branch added a nullable organizationId column to the platform FrameworkEditorFramework / FrameworkEditorRequirement tables so a single row was either platform (null) or org-custom (set). That hybrid shape mismatched the template/instance pattern used everywhere else in the codebase and caused two cross-tenant reads (frameworks.service.findOne, findRequirement) to leak one org's custom requirements to another org on the same framework. Move the org-custom data into dedicated CustomFramework / CustomRequirement tables. FrameworkInstance.frameworkId and RequirementMap.requirementId become nullable and gain customFrameworkId / customRequirementId siblings with DB CHECK constraints enforcing exactly one of the two is set. The editor tables are pure platform definitions again, so the leaks vanish structurally (no shared table to filter) rather than relying on filter discipline in each read. - Schema: revert organizationId from FrameworkEditor tables; add CustomFramework + CustomRequirement; relax/branch FrameworkInstance and RequirementMap FKs with CHECK constraints - Migration: move existing per-org rows into the new tables, repoint FKs, drop the old columns - API: rewrite FrameworksService findOne / findRequirement / createCustom / createRequirement / linkRequirements / linkControlsToRequirement / findAvailable to branch on platform vs custom. Update ControlsService create + linkRequirements + DTOs to accept customRequirementId (with exactly-one validation) and persist documentTypes in the same transaction - Frontend: plumb isCustom through the requirement/control sheets, widen FrameworkInstanceWithControls / FrameworkInstanceForTasks to surface customFramework, and fix all fw.framework.* null-unsafe reads - Tests: frameworks.service.spec regression coverage that a custom FI never reads from frameworkEditorRequirement and a platform FI never reads from customRequirement Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
11 issues found across 40 files (changes from recent commits).
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]/controls/[controlId]/components/DocumentsTable.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/controls/[controlId]/components/DocumentsTable.tsx:116">
P3: Disable all unlink buttons while a request is pending so users don’t click enabled controls that are blocked by `if (pending) return`.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/controls/hooks/useControls.ts">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/controls/hooks/useControls.ts:18">
P2: `requirementMappings` now allows invalid combinations of requirement IDs, but the backend requires exactly one of `requirementId` or `customRequirementId` per mapping.</violation>
</file>
<file name="apps/api/src/controls/controls.controller.ts">
<violation number="1" location="apps/api/src/controls/controls.controller.ts:162">
P2: Validate `formType` with an enum pipe on the route param; the current signature does not enforce runtime enum validation.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/frameworks/page.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/frameworks/page.tsx:32">
P2: This filter only excludes platform frameworks and misses custom framework instances. Already-enabled custom frameworks can still appear as addable.</violation>
</file>
<file name="apps/api/src/policies/policies.controller.ts">
<violation number="1" location="apps/api/src/policies/policies.controller.ts:234">
P2: Filtering to `fi.framework` here causes policy regeneration to silently ignore org custom frameworks. Include `customFramework` instances as well so the AI context reflects all selected frameworks.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/controls/[controlId]/components/documentTypeLabels.ts">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/controls/[controlId]/components/documentTypeLabels.ts:5">
P2: `DOCUMENT_TYPE_LABELS` is typed too loosely (`Record<string, string>`), so this hardcoded list can drift from the canonical evidence-form type set without compile-time errors.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/overview/components/FrameworksOverview.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/overview/components/FrameworksOverview.tsx:85">
P2: Also check `customFramework?.id` when filtering available frameworks; otherwise already-added custom frameworks are treated as still available.</violation>
</file>
<file name="apps/api/src/controls/controls.service.ts">
<violation number="1" location="apps/api/src/controls/controls.service.ts:249">
P1: Validate policy/task ownership by `organizationId` before connecting in `create()`, otherwise a control can be linked to another organization’s records if their IDs are provided.</violation>
<violation number="2" location="apps/api/src/controls/controls.service.ts:262">
P1: `create()` should enforce the same org/framework validation as `linkRequirements()` before writing `requirementMap` rows; otherwise invalid or cross-tenant requirement links can be persisted.</violation>
</file>
<file name="packages/db/prisma/schema/custom-framework.prisma">
<violation number="1" location="packages/db/prisma/schema/custom-framework.prisma:28">
P1: `CustomRequirement` does not enforce tenant consistency between `organizationId` and `customFrameworkId`, allowing cross-org requirement-to-framework links.</violation>
</file>
<file name="apps/api/src/frameworks/frameworks-scores.helper.ts">
<violation number="1" location="apps/api/src/frameworks/frameworks-scores.helper.ts:227">
P1: Use submission timestamp (submittedAt) for evidence recency checks; using createdAt can produce incorrect compliance completion.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
controls.service.create() accepted policyIds, taskIds, and requirementMappings
(frameworkInstanceId, requirementId, customRequirementId) without verifying
they belong to the caller's organization. Prisma FKs only check existence,
not tenancy, so a user with control:create in org A could submit a
frameworkInstanceId from org B — the resulting RequirementMap would join the
attacker's control to the victim's framework, persistently rendering attacker
content inside org B's compliance UI via FrameworksService.findOne /
findRequirement includes.
Validate every FK against organizationId before the transaction, mirroring
the logic already present in linkRequirements:
- policies / tasks: findMany filtered by { id in, organizationId } and
reject if any are missing
- frameworkInstance: must belong to caller org
- requirementId: platform requirement's frameworkId must match the FI's
frameworkId
- customRequirementId: custom requirement must be in caller org and its
customFrameworkId must match the FI's customFrameworkId
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…om-framework parity - Schema: enforce tenant consistency on CustomFramework FKs with composite (id, organizationId) references. CustomRequirement and FrameworkInstance can no longer point at another org's CustomFramework, even if application code regresses. Migration adds a guard that aborts if any existing row already violates the invariant. - Scoring: use EvidenceSubmission.submittedAt (canonical submission time) instead of createdAt for the "within last 6 months" recency check; update all evidenceSubmission selects/orderBys in frameworks.service to match. - Policies (both admin + org): include customFramework when collecting the AI policy-regeneration context so org-custom frameworks influence the prompt instead of being silently dropped. - Frontend framework list: exclude already-added custom frameworks from the "available to add" list on both overview and frameworks pages. - useControls createControl payload: tighten requirementMappings to a discriminated union so invalid requirementId+customRequirementId combos fail at compile time (matches the backend's exactly-one rule). - Controls controller: validate :formType path param with ParseEnumPipe. - Document-type labels map: type as Record<EvidenceFormType, string> so Prisma enum drift becomes a compile error. - DocumentsTable: disable every unlink button while any unlink is pending so users can't click enabled controls that no-op. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
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/controls/controls.service.ts">
<violation number="1" location="apps/api/src/controls/controls.service.ts:306">
P2: Duplicate `policyIds` are incorrectly treated as invalid because validation compares against the non-deduplicated input length.</violation>
<violation number="2" location="apps/api/src/controls/controls.service.ts:321">
P2: Duplicate `taskIds` are incorrectly rejected due to comparing fetched row count with the non-deduplicated input length.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
validatePolicyIds / validateTaskIds compared findMany row count to the raw input length. Duplicate ids in the request (e.g. [p1, p1, p2]) made the input longer than the set of rows returned, so a legitimate request with repeated ids was rejected as invalid. Dedupe via Set before the findMany lookup and length check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@cubic-dev-ai review this |
Resolve bun.lock and package.json conflicts by accepting main.
@Marfuen I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 64 files
Note: This PR contains a large number of files. During the trial, cubic reviews up to 50 files per PR. Paid plans get a higher limit.
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/frameworks/dto/create-custom-framework.dto.ts">
<violation number="1" location="apps/api/src/frameworks/dto/create-custom-framework.dto.ts:13">
P2: `description` is required but allows empty strings. The `name` field correctly uses `@MinLength(1)` to prevent this — add the same guard here, or mark the field `@IsOptional()` if blank descriptions are acceptable.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/requirements/[requirementKey]/components/AddCustomControlSheet.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/requirements/[requirementKey]/components/AddCustomControlSheet.tsx:96">
P2: Form state is not reset when the sheet is dismissed without submitting. If a user partially fills the form, closes the sheet, and reopens it, the previous values and validation errors persist. Reset the form on close.</violation>
</file>
<file name="apps/api/src/frameworks/frameworks-scores.helper.ts">
<violation number="1" location="apps/api/src/frameworks/frameworks-scores.helper.ts:271">
P1: A control with no linked policies, tasks, or document types can never be "completed" due to the `hasAnyArtifact` guard, yet it still counts in the denominator. This permanently lowers the framework score for any unconfigured or intentionally-empty control. Consider either excluding artifact-less controls from the count or treating them as completed.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/controls/[controlId]/components/LinkRequirementForControlSheet.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/frameworks/[frameworkInstanceId]/controls/[controlId]/components/LinkRequirementForControlSheet.tsx:121">
P2: Use a composite React key to avoid duplicates when the same requirement appears across multiple framework instances. `opt.id` is the raw requirement ID, which can repeat if the same framework is instantiated more than once. Use `\`${opt.frameworkInstanceId}-${opt.id}\`` instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
|
|
||
| return Math.round(((policyRatio + taskRatio) / 2) * 100); | ||
| const hasAnyArtifact = | ||
| policies.length > 0 || controlTasks.length > 0 || documentTypes.length > 0; |
There was a problem hiding this comment.
P1: A control with no linked policies, tasks, or document types can never be "completed" due to the hasAnyArtifact guard, yet it still counts in the denominator. This permanently lowers the framework score for any unconfigured or intentionally-empty control. Consider either excluding artifact-less controls from the count or treating them as completed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/frameworks/frameworks-scores.helper.ts, line 271:
<comment>A control with no linked policies, tasks, or document types can never be "completed" due to the `hasAnyArtifact` guard, yet it still counts in the denominator. This permanently lowers the framework score for any unconfigured or intentionally-empty control. Consider either excluding artifact-less controls from the count or treating them as completed.</comment>
<file context>
@@ -219,40 +222,68 @@ interface TaskWithControls {
- return Math.round(((policyRatio + taskRatio) / 2) * 100);
+ const hasAnyArtifact =
+ policies.length > 0 || controlTasks.length > 0 || documentTypes.length > 0;
+
+ return (
</file context>
|
|
||
| @ApiProperty({ description: 'Framework description' }) | ||
| @IsString() | ||
| @MaxLength(2000) |
There was a problem hiding this comment.
P2: description is required but allows empty strings. The name field correctly uses @MinLength(1) to prevent this — add the same guard here, or mark the field @IsOptional() if blank descriptions are acceptable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/frameworks/dto/create-custom-framework.dto.ts, line 13:
<comment>`description` is required but allows empty strings. The `name` field correctly uses `@MinLength(1)` to prevent this — add the same guard here, or mark the field `@IsOptional()` if blank descriptions are acceptable.</comment>
<file context>
@@ -0,0 +1,21 @@
+
+ @ApiProperty({ description: 'Framework description' })
+ @IsString()
+ @MaxLength(2000)
+ description: string;
+
</file context>
| > | ||
| Add Control | ||
| </Button> | ||
| <Sheet open={isOpen} onOpenChange={setIsOpen}> |
There was a problem hiding this comment.
P2: Form state is not reset when the sheet is dismissed without submitting. If a user partially fills the form, closes the sheet, and reopens it, the previous values and validation errors persist. Reset the form on close.
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]/requirements/[requirementKey]/components/AddCustomControlSheet.tsx, line 96:
<comment>Form state is not reset when the sheet is dismissed without submitting. If a user partially fills the form, closes the sheet, and reopens it, the previous values and validation errors persist. Reset the form on close.</comment>
<file context>
@@ -0,0 +1,149 @@
+ >
+ Add Control
+ </Button>
+ <Sheet open={isOpen} onOpenChange={setIsOpen}>
+ <SheetContent>
+ <SheetHeader>
</file context>
| ) : ( | ||
| <div className="space-y-2"> | ||
| {options.map((opt) => ( | ||
| <label |
There was a problem hiding this comment.
P2: Use a composite React key to avoid duplicates when the same requirement appears across multiple framework instances. opt.id is the raw requirement ID, which can repeat if the same framework is instantiated more than once. Use \${opt.frameworkInstanceId}-${opt.id}`` instead.
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]/controls/[controlId]/components/LinkRequirementForControlSheet.tsx, line 121:
<comment>Use a composite React key to avoid duplicates when the same requirement appears across multiple framework instances. `opt.id` is the raw requirement ID, which can repeat if the same framework is instantiated more than once. Use `\`${opt.frameworkInstanceId}-${opt.id}\`` instead.</comment>
<file context>
@@ -0,0 +1,157 @@
+ ) : (
+ <div className="space-y-2">
+ {options.map((opt) => (
+ <label
+ key={opt.id}
+ className="flex items-start gap-3 rounded border p-3 cursor-pointer hover:bg-muted/40"
</file context>
|
🎉 This PR is included in version 3.25.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This is an automated pull request to merge lewis/comp-framework-controls into dev.
It was created by the [Auto Pull Request] action.
Summary by cubic
Adds per‑org custom frameworks and requirements with strict tenant enforcement. Adds control linking for policies, tasks, requirements, and required evidence document types; scoring now respects required document types and uses evidence submission time.
New Features
POST /v1/frameworks/custom,POST /v1/frameworks/:id/requirements,POST /v1/frameworks/:id/requirements/link,POST /v1/frameworks/:id/requirements/:requirementKey/controls/link;POST /v1/controls/:id/policies/link,POST /v1/controls/:id/tasks/link,POST /v1/controls/:id/requirements/link,POST /v1/controls/:id/document-types/link,GET /v1/controls/options. Available frameworks, onboarding lists, and policy prompts include org‑custom; already‑added custom frameworks are excluded from “add” lists.submittedAt; link endpoints return DB counts; dedupepolicyIds/taskIdsbefore validation.Migration
CustomFrameworkandCustomRequirement;FrameworkInstanceandRequirementMapreference platform or custom (DB CHECK ensures exactly one).Written for commit 2ffc0bd. Summary will update on new commits.