improvement(soa): persist applicability and justification on the SoA answer#3395
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Make Statement of Applicability answers first-class per-document records: each control's applicability (Yes/No) and justification are stored on the organization's own SOAAnswer, alongside the existing answer text and versioning. SOAFrameworkConfiguration stays a shared template (control list, columns, objectives). - add SOAAnswer.isApplicable (+ migration) - auto-fill and manual save persist applicability + justification on the answer - export and the answered-count read from the document's own answers - keep the justification for both applicable and not-applicable controls - desktop/mobile rows read from the document's answers via a shared resolveSoaDisplay helper Adds API and frontend unit tests.
tofikwest
force-pushed
the
tofik/cs-731-soa-per-org-answers
branch
from
2d75ca3 to
2394dca
Compare
tofikwest
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
2 issues found and verified against the latest diff
Confidence score: 3/5
- The biggest risk is in
packages/db/prisma/migrations/20260713120000_add_soa_answer_is_applicable/migration.sqltogether withresolveSoaDisplay.ts: legacySOAAnswerrows can end up withisApplicable = NULL, and the new read path no longer falls back to shared config, so existing statements may render with incorrect applicability or missing answers after deploy — add a migration backfill/default (or restore a fallback path) before merging. - In
apps/app/src/app/(app)/[orgId]/documents/statement-of-applicability/components/soa-display.ts, introducing another localProcessedResult-equivalent type alongsideSOATableRow.tsx,SOATable.tsx, andSOAMobileRow.tsxincreases drift risk, where one view updates while others silently diverge in behavior — consolidate to a single shared type to de-risk future changes.
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]/documents/statement-of-applicability/components/soa-display.ts">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/documents/statement-of-applicability/components/soa-display.ts:3">
P2: The new `SOAProcessedResult` type duplicates identical local `ProcessedResult` types already defined in `SOATableRow.tsx`, `SOATable.tsx`, and `SOAMobileRow.tsx`. Keeping four independent definitions means any future change to the processed-result contract (e.g., adding a new flag or changing optionality) can be updated in one place but missed in the others, reintroducing the drift this helper was meant to prevent.
You already export `SOAProcessedResult` from this file; the consuming components should import it and delete their local `ProcessedResult` declarations so the display contract is defined once.</violation>
</file>
<file name="packages/db/prisma/migrations/20260713120000_add_soa_answer_is_applicable/migration.sql">
<violation number="1" location="packages/db/prisma/migrations/20260713120000_add_soa_answer_is_applicable/migration.sql:3">
P1: Legacy SOAAnswer rows receive `isApplicable = NULL` from this migration, and the new per-answer read paths no longer fall back to the shared configuration. In `resolveSoaDisplay.ts` a NULL `isApplicable` falls through to `displayIsApplicable: true`, while `exportDocument` emits `isApplicable: null`. This means existing completed documents will silently display/export incorrect applicability values—previously-applicable=false controls become YES—until each organization manually re-runs auto-fill. Since this PR is fixing cross-tenant data leakage (CS-731), consider adding a backfill or at minimum a post-deploy enforcement step so legacy documents are not left in an incorrect state.</violation>
</file>
Linked issue analysis
Linked issue: CS-731: "[Bug] Another org's name ('Lexcom') appears throughout BrandQuantum's generated ISO 27001 Statement of Applicability"
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Investigate how another org's name/details ended up in BrandQuantum's generated SoA (is there context/template bleed between orgs?). | PR documents the root cause (applicability/justifications were stored on a shared SOAFrameworkConfiguration row keyed only by frameworkId, and writes from autofill/manual save persisted into that shared row), and removes the code paths that wrote per-org data into the shared configuration. |
| ✅ | Confirm whether this affects other orgs (i.e. is Lexcom's content appearing elsewhere?). | PR explicitly states the blast radius and implements fixes treating the problem as cross-tenant; tests and code changes demonstrate the shared configuration was reused by every org and would cause last-writer bleed across tenants. |
| Correct BrandQuantum's SoA justifications to reflect BrandQuantum, and confirm the content is regenerated from their actual context. | The PR fixes the data model and code paths so future exports and frontend displays are driven by per-organization SOAAnswer rows (and auto-fill/manual saves now persist per-org isApplicable/justification). However, the PR does not perform the operational regeneration for existing documents in prod — the remediation notes instruct affected orgs to re-run Auto-fill to repopulate per-org applicability. That operational step is intentionally out-of-band (not included in this PR). |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| @@ -0,0 +1,3 @@ | |||
| -- Store the Statement of Applicability applicability decision per organization | |||
| -- (on the answer) instead of on the shared, framework-level configuration. | |||
| ALTER TABLE "SOAAnswer" ADD COLUMN "isApplicable" BOOLEAN; | |||
There was a problem hiding this comment.
P1: Legacy SOAAnswer rows receive isApplicable = NULL from this migration, and the new per-answer read paths no longer fall back to the shared configuration. In resolveSoaDisplay.ts a NULL isApplicable falls through to displayIsApplicable: true, while exportDocument emits isApplicable: null. This means existing completed documents will silently display/export incorrect applicability values—previously-applicable=false controls become YES—until each organization manually re-runs auto-fill. Since this PR is fixing cross-tenant data leakage (CS-731), consider adding a backfill or at minimum a post-deploy enforcement step so legacy documents are not left in an incorrect state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/prisma/migrations/20260713120000_add_soa_answer_is_applicable/migration.sql, line 3:
<comment>Legacy SOAAnswer rows receive `isApplicable = NULL` from this migration, and the new per-answer read paths no longer fall back to the shared configuration. In `resolveSoaDisplay.ts` a NULL `isApplicable` falls through to `displayIsApplicable: true`, while `exportDocument` emits `isApplicable: null`. This means existing completed documents will silently display/export incorrect applicability values—previously-applicable=false controls become YES—until each organization manually re-runs auto-fill. Since this PR is fixing cross-tenant data leakage (CS-731), consider adding a backfill or at minimum a post-deploy enforcement step so legacy documents are not left in an incorrect state.</comment>
<file context>
@@ -0,0 +1,3 @@
+-- Store the Statement of Applicability applicability decision per organization
+-- (on the answer) instead of on the shared, framework-level configuration.
+ALTER TABLE "SOAAnswer" ADD COLUMN "isApplicable" BOOLEAN;
</file context>
| @@ -0,0 +1,79 @@ | |||
| import type { SOATableAnswerData } from './soa-field-types'; | |||
|
|
|||
| export type SOAProcessedResult = { | |||
There was a problem hiding this comment.
P2: The new SOAProcessedResult type duplicates identical local ProcessedResult types already defined in SOATableRow.tsx, SOATable.tsx, and SOAMobileRow.tsx. Keeping four independent definitions means any future change to the processed-result contract (e.g., adding a new flag or changing optionality) can be updated in one place but missed in the others, reintroducing the drift this helper was meant to prevent.
You already export SOAProcessedResult from this file; the consuming components should import it and delete their local ProcessedResult declarations so the display contract is defined once.
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]/documents/statement-of-applicability/components/soa-display.ts, line 3:
<comment>The new `SOAProcessedResult` type duplicates identical local `ProcessedResult` types already defined in `SOATableRow.tsx`, `SOATable.tsx`, and `SOAMobileRow.tsx`. Keeping four independent definitions means any future change to the processed-result contract (e.g., adding a new flag or changing optionality) can be updated in one place but missed in the others, reintroducing the drift this helper was meant to prevent.
You already export `SOAProcessedResult` from this file; the consuming components should import it and delete their local `ProcessedResult` declarations so the display contract is defined once.</comment>
<file context>
@@ -0,0 +1,79 @@
+import type { SOATableAnswerData } from './soa-field-types';
+
+export type SOAProcessedResult = {
+ success: boolean;
+ isApplicable: boolean | null;
</file context>
- resolveSoaDisplay now shows an answer that has no applicability value yet as unanswered (N/A) — consistent with the export — instead of assuming the control is applicable, so pre-existing documents don't render a value that was never set for them. - Consolidate the per-view processed-result types into a single shared SOAProcessedResult (used by the autofill hook and every SoA view) so the contract is defined once.
|
Thanks @cubic-dev-ai — both addressed in c6aebdb: P1 (applicability of pre-existing answers). Good catch on the inconsistency: the table defaulted an answer with no applicability value to Yes while the export rendered N/A. A migration backfill isn't possible here — applicability was not previously stored on the answer, so there's no reliable per-answer value to populate — so the correct fix is to make the read paths consistent and honest: P2 (duplicated processed-result type). Consolidated into a single |
@tofikwest Both look right to me. P1 — The pre-migration answer path was the right call. Since P2 — The type consolidation cleaned up four separate No further concerns from my side on these two. Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
|
@cubic-dev-ai review it |
@tofikwest I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
cubic analysis
1 issue found across 17 files
Confidence score: 3/5
- In
apps/api/src/soa/utils/soa-storage.ts, the per-organizationcountAnsweredAnswerslogic can over/undercount answered items because it relies onSOAAnswer.questionIdstrings that are not constrained to the document’s current question configuration, which can skew completion/progress metrics after merge; before merging, scope the count to valid question IDs for the active configuration (or enforce referential integrity) and add a regression test for stale/mismatchedquestionIddata.
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/soa/utils/soa-storage.ts">
<violation number="1" location="apps/api/src/soa/utils/soa-storage.ts:108">
P1: The new per-organization `countAnsweredAnswers` counts every latest `SOAAnswer` with a non-null `isApplicable`, but `questionId` is only a plain string with no foreign-key constraint to the document's configuration questions, and `saveAnswer` does not verify that the submitted `questionId` exists in the document's configuration before creating a row. This means a caller can create applicability-bearing answers for arbitrary question IDs, inflating `answeredQuestions` and incorrectly driving document status to `completed` while real controls remain unanswered.
To fix this, validate `dto.questionId` against the document's `configuration.questions` array in `saveAnswer` before creating an answer, and also scope `countAnsweredAnswers` to count only rows whose `questionId` matches a control in the document's configuration.</violation>
</file>
Linked issue analysis
Linked issue: CS-731: "[Bug] Another org's name ('Lexcom') appears throughout BrandQuantum's generated ISO 27001 Statement of Applicability"
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | SoA export and UI read applicability + justification from the organization’s own answers (not from the shared SOAFrameworkConfiguration) | Service/export and frontend display were changed to source isApplicable/justification from per-document SOAAnswer (not the shared configuration); tests verify exports use the org's answers over shared-config values. |
| ✅ | Persist per-answer applicability (add isApplicable column and DB migration) | Schema and migration add the isApplicable column and code stores/reads it from SOAAnswer; tests exercise saveAnswer behavior. |
| ✅ | Stop writing per-organization applicability/justification into the shared framework configuration (prevent template bleed) | Removed updateConfigurationWithResults logic and related helpers; tests assert the framework configuration is not updated with per-org data. |
| ✅ | Investigate / confirm whether other orgs were affected by the bleed (root-cause investigation) | Tests and code comments demonstrate the previous shared-configuration values could bleed into every org's export and the PR's changes close that vector; this constitutes an investigation and confirmation of the bleed behavior. |
| Correct existing BrandQuantum SoA justifications and confirm content is regenerated from BrandQuantum’s actual context | The PR ensures future exports and UI display reflect per-org answers and documents a rollout path to repair pre-migration documents (re-run Auto-fill or edit controls). However, it does not include an automated migration/one‑time job that proactively re-runs Auto-fill for existing documents — correction for existing BrandQuantum SoAs requires an explicit re-run of Auto-fill or manual edits as stated in the rollout note. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| */ | ||
| export async function getAnsweredCountFromConfiguration( | ||
| configurationId: string, | ||
| export async function countAnsweredAnswers( |
There was a problem hiding this comment.
P1: The new per-organization countAnsweredAnswers counts every latest SOAAnswer with a non-null isApplicable, but questionId is only a plain string with no foreign-key constraint to the document's configuration questions, and saveAnswer does not verify that the submitted questionId exists in the document's configuration before creating a row. This means a caller can create applicability-bearing answers for arbitrary question IDs, inflating answeredQuestions and incorrectly driving document status to completed while real controls remain unanswered.
To fix this, validate dto.questionId against the document's configuration.questions array in saveAnswer before creating an answer, and also scope countAnsweredAnswers to count only rows whose questionId matches a control in the document's configuration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/soa/utils/soa-storage.ts, line 108:
<comment>The new per-organization `countAnsweredAnswers` counts every latest `SOAAnswer` with a non-null `isApplicable`, but `questionId` is only a plain string with no foreign-key constraint to the document's configuration questions, and `saveAnswer` does not verify that the submitted `questionId` exists in the document's configuration before creating a row. This means a caller can create applicability-bearing answers for arbitrary question IDs, inflating `answeredQuestions` and incorrectly driving document status to `completed` while real controls remain unanswered.
To fix this, validate `dto.questionId` against the document's `configuration.questions` array in `saveAnswer` before creating an answer, and also scope `countAnsweredAnswers` to count only rows whose `questionId` matches a control in the document's configuration.</comment>
<file context>
@@ -137,25 +102,19 @@ export async function updateDocumentAfterAutoFill(
*/
-export async function getAnsweredCountFromConfiguration(
- configurationId: string,
+export async function countAnsweredAnswers(
+ documentId: string,
): Promise<number> {
</file context>
- preserve prior applicability/justification when a field is omitted from a save, so a partial edit can't wipe a previously set decision - validate that questionId belongs to the document's configuration before creating an answer, keeping the per-org answered-count accurate - require a justification when a control is marked not applicable - hydrate the answers map through a single mapper so the sync effect no longer sets savedIsApplicable (which would shadow in-session autofill results)
|
Thanks @cubic-dev-ai — all four addressed in 788bb77: P1 — sync effect set P1 — omitted P1 — P2 — NO without justification. Added service tests for the invalid-questionId, NO-without-justification, and preserve-on-omit cases (API SoA suite: 58 passing; app SoA suite: 13 passing). |
Those all look good to me. A few quick observations: Merge semantics on partial save — The
Question ID validation — NO + blank justification — The guard at No issues from my side. Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
…ions countAnsweredAnswers now counts only latest answered rows whose questionId belongs to the document's active configuration, so stale or mismatched answer rows can't skew completion/progress metrics. Both callers (manual save and auto-fill) pass the configured question IDs. Adds a regression test.
|
Thanks @cubic-dev-ai — done in 56b4132. In addition to the write-path validation added earlier ( where: {
documentId,
isLatestAnswer: true,
isApplicable: { not: null },
questionId: { in: validQuestionIds },
}Both callers (manual save and auto-fill) pass the configured IDs, so stale or mismatched answer rows — e.g. an answer for a control later removed from the configuration — can no longer inflate the completion count. Added |
|
@cubic-dev-ai review it |
@tofikwest I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
cubic analysis
All reported issues were addressed across 18 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Linked issue analysis
Linked issue: CS-731: "[Bug] Another org's name ('Lexcom') appears throughout BrandQuantum's generated ISO 27001 Statement of Applicability"
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | SoA justifications are sourced from the organization's own answers (no shared-framework bleed) | Service, storage, export and frontend code were changed to read applicability/justification from SOAAnswer (per-document/org) instead of the shared SOAFrameworkConfiguration. Tests assert the export and UI use the org's answers rather than shared config. |
| ✅ | Frontend and export/answered-count behavior updated so future SoAs are generated from the org's context | Answered-count now counts per-org latest answers scoped to the document's question IDs; frontend reads applicability/justification from document answers or in-session autofill, and export uses those answers. |
| Investigation: confirm whether other orgs' SoAs (e.g. Lexcom) were affected | The PR demonstrates how bleed could happen (shared config containing other-org justification) and adds tests showing the problem and the fix, but it does not include a database audit or script that enumerates which org documents were actually affected in production. | |
| Correct BrandQuantum's SoA justifications and regenerate content for that org | The code and schema changes enable correct regeneration: after this change, re-running Auto-fill or editing a control will populate per-answer applicability/justification for pre-existing documents. However, the PR does not include an automated one-off migration or an executed re-run for BrandQuantum's document — remediation is manual (re-run Auto-fill) per the rollout note. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Remove the display-only override that forced physical-security (7.x) controls to "not applicable" for fully-remote orgs on screen. The rule is already applied at generation time (auto-fill persists isApplicable=false with a justification and the field is edit-locked), so the persisted answer already reflects it. Reading it from there keeps the on-screen SoA and the exported PDF in agreement instead of applying a rule in one channel but not the other.
|
Thanks @cubic-dev-ai — good catch, fixed in bd7aaa7 by removing the frontend-only override so both channels read the same source. The "fully remote → physical-security (7.x) controls are not applicable" rule is already enforced at the data layer: auto-fill persists I chose removing the override over re-applying the rule in the export + answered-count because the latter would duplicate the same rule across four places (display, export, count, generation) and force the export service to look up the org's fully-remote status — the drift risk this review has been guarding against. Enforcing it once at generation and reading the persisted value everywhere keeps a single source of truth. Updated the helper's tests accordingly (app SoA suite: 12 passing). |
@tofikwest That reasoning is sound. The fully-remote rule was applied in two places (generation and display), which meant they could drift — and they already did, since the export was always reading the persisted answer while the screen had the display-only override. Now both paths converge on the same persisted value. The key insight here is that the rule is an input to the decision, not an output property. It gets applied once at generation time (auto-fill), persisted as The alternative (re-applying in export + count) would have introduced the exact kind of duplication you're eliminating here: the rule would live in four places, and a change to the scoping logic (say, a control outside 7.x becomes not-applicable for remote orgs) would need coordinated updates across all of them. Single source of truth is the right call. The diff confirms the override is gone from both Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
|
@cubic-dev-ai review it |
@tofikwest I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
cubic analysis
All reported issues were addressed across 18 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Linked issue analysis
Linked issue: CS-731: "[Bug] Another org's name ('Lexcom') appears throughout BrandQuantum's generated ISO 27001 Statement of Applicability"
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Persist applicability + justification on each organization's SOAAnswer (DB + schema + service) | PR adds isApplicable to the SOAAnswer model, a migration, and save/export paths that persist and read isApplicable and justification per-document. |
| ✅ | Ensure exports and the UI derive SoA content from the document's own answers rather than the shared framework configuration | Export mapping and UI display now source applicability/justification from the document answers; the resolveSoaDisplay helper and updated export logic prevent reading stale shared-config justifications. |
| ✅ | Stop writing per-organization data into the shared SOAFrameworkConfiguration (prevent template bleed) | Code that previously updated the shared configuration has been removed and tests assert the shared configuration is not written to during saves/auto-fill. |
| ✅ | Investigate root cause and confirm scope (i.e. other orgs affected) | PR contains tests and comments that demonstrate the previous behavior (shared-config values could bleed into exports) and now asserts exports are sourced from the org's own answers, which confirms and addresses cross-org bleed. |
| Correct BrandQuantum's SoA justifications and confirm content is regenerated for that org | PR provides the necessary migration and ensures future/ regenerated exports will be correct; however it does not itself re-run auto-fill or modify existing BrandQuantum SoA documents — the rollout note says re-run Auto‑fill (or edit) to populate the new per-answer applicability field. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
saveAnswer previously retired the prior answer (isLatestAnswer=false) before running the not-applicable-requires-justification check, so a rejected save could leave a control with no latest answer. Move all validation ahead of any write, and wrap the retire-previous + create-new steps in a single transaction so a failure can never leave a control without a latest answer. Apply the same atomic retire+create in the auto-fill save path. Adds a regression test asserting a failed validation does not retire the prior answer.
|
Thanks @cubic-dev-ai — real bug, fixed in 2cf26d6.
I also did a full pass over the diff for the same class of issue: those are the only two retire-then-create sites, both now atomic; all validation precedes writes; and no per-org data is read from the shared configuration anywhere. |
@tofikwest Looks solid from the diff. A few things that stand out as exactly right: Validation-first ordering — In Atomic retire+create — Both Both paths consistent — I checked: No shared-config writes — 61 passing and the regression test covers the hole — looks complete. Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
tofikwest
changed the title
|
🎉 This PR is included in version 3.101.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
Summary
Makes Statement of Applicability answers first-class, per-document records. Each control's applicability (Yes/No) and justification are now stored on the organization's own
SOAAnswerfor that document, alongside the existing answer text and versioning.SOAFrameworkConfigurationstays a shared template (control list, columns, objectives).As a result, each organization's SoA — on screen, in the answered-count, and in the exported PDF — is derived entirely from that organization's own answers.
What changed
Data model
SOAAnswer.isApplicablecolumn (+ migration20260713120000_add_soa_answer_is_applicable) so applicability lives with the answer and its justification.API (
apps/api/src/soa)SOAAnswer.Frontend (
apps/app/.../statement-of-applicability)resolveSoaDisplayhelper that removes duplicated display logic across the two rows.Rollout note
Existing SoAs surface their own justifications immediately. To populate the new per-answer applicability field for a SoA generated before this change, re-run Auto-fill once (or edit the control); newly generated or edited SoAs are complete with no extra step.
Testing
apps/api:npx jest src/soa— 55 passing (incl. export sourcing from the org's own answers).apps/app:npx vitest run .../statement-of-applicability— 12 passing (incl. newsoa-display.test.ts).