Skip to content

Commit bd43e8a

Browse files
Marfuenclaude
andauthored
fix: address cubic review findings on onboarding PR (#2783)
* fix: address cubic review findings on onboarding PR P1: Add metadata.set('policies', true) after policy fan-out so the tracker boolean flag is set. P1: Log batchTriggerAndWait failures in vendor/risk mitigation fan-outs instead of silently ignoring them. P2: Strip {{#if}}/{{/if}} markers from mixed-content nodes so template syntax doesn't leak into rendered policies. P2: Fix stale onboardingTriggerJobId locking publish button in ToDoOverview. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: strip only first marker occurrence to preserve nested conditionals The global regex flag in stripMarkerText would remove ALL matching {{#if}}/{{/if}} markers in a subtree, corrupting boundaries of nested conditional blocks. Removed the g flag so only the first occurrence (the one that triggered the match) is stripped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 26e1667 commit bd43e8a

6 files changed

Lines changed: 358 additions & 6 deletions

File tree

apps/api/src/frameworks/frameworks-scores.helper.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ export async function getOverviewScores(organizationId: string) {
2929
}),
3030
db.onboarding.findUnique({
3131
where: { organizationId },
32-
select: { triggerJobId: true },
32+
select: { triggerJobId: true, triggerJobCompleted: true },
3333
}),
3434
db.organization.findUnique({
3535
where: { id: organizationId },
@@ -90,7 +90,7 @@ export async function getOverviewScores(organizationId: string) {
9090
incompleteTasks,
9191
},
9292
people,
93-
onboardingTriggerJobId: onboarding?.triggerJobId ?? null,
93+
onboardingTriggerJobId: onboarding?.triggerJobCompleted ? null : (onboarding?.triggerJobId ?? null),
9494
documents: await computeDocumentsScore(organizationId),
9595
findings: await getOrganizationFindings(organizationId),
9696
};

apps/app/src/trigger/tasks/onboarding/generate-risk-mitigation.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ export const generateRiskMitigationsForOrg = task({
114114

115115
const policies = policyRows.map((p) => ({ name: p.name, description: p.description }));
116116

117-
await tasks.batchTriggerAndWait<typeof generateRiskMitigation>(
117+
const batchResult = await tasks.batchTriggerAndWait<typeof generateRiskMitigation>(
118118
'generate-risk-mitigation',
119119
risks.map((r) => ({
120120
payload: {
@@ -126,6 +126,12 @@ export const generateRiskMitigationsForOrg = task({
126126
options: { concurrencyKey: `${organizationId}:${r.id}` },
127127
})),
128128
);
129+
const failures = batchResult.runs.filter((r) => !r.ok);
130+
if (failures.length > 0) {
131+
logger.error(`${failures.length} risk mitigation(s) failed`, {
132+
failedRunIds: failures.map((r) => r.id),
133+
});
134+
}
129135

130136
// Revalidate the parent risk routes after batch triggering
131137
try {

apps/app/src/trigger/tasks/onboarding/generate-vendor-mitigation.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ export const generateVendorMitigationsForOrg = task({
116116

117117
const policies = policyRows.map((p) => ({ name: p.name, description: p.description }));
118118

119-
await tasks.batchTriggerAndWait<typeof generateVendorMitigation>(
119+
const batchResult = await tasks.batchTriggerAndWait<typeof generateVendorMitigation>(
120120
'generate-vendor-mitigation',
121121
vendors.map((v) => ({
122122
payload: {
@@ -128,6 +128,12 @@ export const generateVendorMitigationsForOrg = task({
128128
options: { concurrencyKey: `${organizationId}:${v.id}` },
129129
})),
130130
);
131+
const failures = batchResult.runs.filter((r) => !r.ok);
132+
if (failures.length > 0) {
133+
logger.error(`${failures.length} vendor mitigation(s) failed`, {
134+
failedRunIds: failures.map((r) => r.id),
135+
});
136+
}
131137

132138
// Revalidate the parent vendors route after batch triggering
133139
try {

apps/app/src/trigger/tasks/onboarding/onboard-organization.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ export const onboardOrganization = task({
106106
const policyCount = policyList.length;
107107
metadata.set('currentStep', `Tailoring Policies... (0/${policyCount})`);
108108
await updateOrganizationPolicies(payload.organizationId, questionsAndAnswers, frameworks);
109+
metadata.set('policies', true);
109110

110111
// Extract vendors + risks in parallel (both are independent LLM calls).
111112
metadata.set('currentStep', 'Creating Vendors...');
Lines changed: 324 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,324 @@
1+
import { describe, it, expect } from 'vitest';
2+
import { processContentArray, buildFlags, buildVariables, processTemplate } from './process-policy-template';
3+
4+
const vars = { COMPANY: 'Acme Inc', EMPLOYEES: '50', DATA: 'PII' };
5+
6+
function textNode(text: string) {
7+
return { type: 'text', text };
8+
}
9+
10+
function paragraph(...children: Record<string, unknown>[]) {
11+
return { type: 'paragraph', content: children };
12+
}
13+
14+
describe('processContentArray', () => {
15+
describe('placeholder replacement', () => {
16+
it('replaces {{COMPANY}} in text nodes', () => {
17+
const nodes = [paragraph(textNode('Welcome to {{COMPANY}}'))];
18+
const result = processContentArray(nodes, vars, {});
19+
expect((result[0] as any).content[0].text).toBe('Welcome to Acme Inc');
20+
});
21+
22+
it('replaces multiple placeholders', () => {
23+
const nodes = [paragraph(textNode('{{COMPANY}} has {{EMPLOYEES}} employees handling {{DATA}}'))];
24+
const result = processContentArray(nodes, vars, {});
25+
expect((result[0] as any).content[0].text).toBe('Acme Inc has 50 employees handling PII');
26+
});
27+
28+
it('replaces unknown placeholders with N/A', () => {
29+
const nodes = [paragraph(textNode('Contact {{UNKNOWN}}'))];
30+
const result = processContentArray(nodes, vars, {});
31+
expect((result[0] as any).content[0].text).toBe('Contact N/A');
32+
});
33+
});
34+
35+
describe('inline conditionals (same text node)', () => {
36+
it('keeps content when flag is true', () => {
37+
const nodes = [paragraph(textNode('Before {{#if soc2}}SOC 2 content{{/if}} after'))];
38+
const result = processContentArray(nodes, vars, { soc2: true });
39+
expect((result[0] as any).content[0].text).toBe('Before SOC 2 content after');
40+
});
41+
42+
it('removes content when flag is false', () => {
43+
const nodes = [paragraph(textNode('Before {{#if hipaa}}HIPAA content{{/if}} after'))];
44+
const result = processContentArray(nodes, vars, { hipaa: false });
45+
expect((result[0] as any).content[0].text).toBe('Before after');
46+
});
47+
48+
it('handles multiple inline conditionals in same text', () => {
49+
const nodes = [paragraph(textNode('{{#if soc2}}SOC2{{/if}} and {{#if hipaa}}HIPAA{{/if}}'))];
50+
const result = processContentArray(nodes, vars, { soc2: true, hipaa: false });
51+
expect((result[0] as any).content[0].text).toBe('SOC2 and ');
52+
});
53+
});
54+
55+
describe('multi-node conditionals (marker-only nodes)', () => {
56+
it('keeps block when flag is true', () => {
57+
const nodes = [
58+
paragraph(textNode('{{#if soc2}}')),
59+
paragraph(textNode('SOC 2 specific content')),
60+
paragraph(textNode('{{/if}}')),
61+
paragraph(textNode('Always visible')),
62+
];
63+
const result = processContentArray(nodes, vars, { soc2: true });
64+
expect(result).toHaveLength(2);
65+
expect((result[0] as any).content[0].text).toBe('SOC 2 specific content');
66+
expect((result[1] as any).content[0].text).toBe('Always visible');
67+
});
68+
69+
it('removes block when flag is false', () => {
70+
const nodes = [
71+
paragraph(textNode('{{#if hipaa}}')),
72+
paragraph(textNode('HIPAA specific content')),
73+
paragraph(textNode('More HIPAA content')),
74+
paragraph(textNode('{{/if}}')),
75+
paragraph(textNode('Always visible')),
76+
];
77+
const result = processContentArray(nodes, vars, { hipaa: false });
78+
expect(result).toHaveLength(1);
79+
expect((result[0] as any).content[0].text).toBe('Always visible');
80+
});
81+
82+
it('removes block for unknown flags (defaults to false)', () => {
83+
const nodes = [
84+
paragraph(textNode('{{#if unknownFramework}}')),
85+
paragraph(textNode('Should be removed')),
86+
paragraph(textNode('{{/if}}')),
87+
];
88+
const result = processContentArray(nodes, vars, {});
89+
expect(result).toHaveLength(0);
90+
});
91+
});
92+
93+
describe('mixed content nodes (marker + text on same node)', () => {
94+
it('strips {{#if}} marker but keeps remaining text when true', () => {
95+
const nodes = [
96+
paragraph(textNode('{{#if soc2}} SOC 2 intro text')),
97+
paragraph(textNode('More content')),
98+
paragraph(textNode('{{/if}}')),
99+
];
100+
const result = processContentArray(nodes, vars, { soc2: true });
101+
expect(result).toHaveLength(2);
102+
expect((result[0] as any).content[0].text).toBe(' SOC 2 intro text');
103+
expect((result[1] as any).content[0].text).toBe('More content');
104+
});
105+
106+
it('strips {{/if}} marker but keeps remaining text', () => {
107+
const nodes = [
108+
paragraph(textNode('{{#if soc2}}')),
109+
paragraph(textNode('Content here')),
110+
paragraph(textNode('End of section {{/if}}')),
111+
];
112+
const result = processContentArray(nodes, vars, { soc2: true });
113+
expect(result).toHaveLength(2);
114+
expect((result[0] as any).content[0].text).toBe('Content here');
115+
expect((result[1] as any).content[0].text).toBe('End of section ');
116+
});
117+
118+
it('removes mixed content node when flag is false', () => {
119+
const nodes = [
120+
paragraph(textNode('{{#if hipaa}} HIPAA intro')),
121+
paragraph(textNode('HIPAA body')),
122+
paragraph(textNode('{{/if}}')),
123+
];
124+
const result = processContentArray(nodes, vars, { hipaa: false });
125+
expect(result).toHaveLength(0);
126+
});
127+
});
128+
129+
describe('nested conditionals', () => {
130+
it('outer true, inner true: keeps both', () => {
131+
const nodes = [
132+
paragraph(textNode('{{#if soc2}}')),
133+
paragraph(textNode('SOC 2 content')),
134+
paragraph(textNode('{{#if hipaa}}')),
135+
paragraph(textNode('SOC 2 + HIPAA content')),
136+
paragraph(textNode('{{/if}}')),
137+
paragraph(textNode('{{/if}}')),
138+
];
139+
const result = processContentArray(nodes, vars, { soc2: true, hipaa: true });
140+
expect(result).toHaveLength(2);
141+
expect((result[0] as any).content[0].text).toBe('SOC 2 content');
142+
expect((result[1] as any).content[0].text).toBe('SOC 2 + HIPAA content');
143+
});
144+
145+
it('outer true, inner false: keeps outer, removes inner', () => {
146+
const nodes = [
147+
paragraph(textNode('{{#if soc2}}')),
148+
paragraph(textNode('SOC 2 only')),
149+
paragraph(textNode('{{#if hipaa}}')),
150+
paragraph(textNode('Should be removed')),
151+
paragraph(textNode('{{/if}}')),
152+
paragraph(textNode('Still SOC 2')),
153+
paragraph(textNode('{{/if}}')),
154+
];
155+
const result = processContentArray(nodes, vars, { soc2: true, hipaa: false });
156+
expect(result).toHaveLength(2);
157+
expect((result[0] as any).content[0].text).toBe('SOC 2 only');
158+
expect((result[1] as any).content[0].text).toBe('Still SOC 2');
159+
});
160+
161+
it('outer false: removes everything including true inner', () => {
162+
const nodes = [
163+
paragraph(textNode('{{#if hipaa}}')),
164+
paragraph(textNode('HIPAA content')),
165+
paragraph(textNode('{{#if soc2}}')),
166+
paragraph(textNode('LEAKED if buggy')),
167+
paragraph(textNode('{{/if}}')),
168+
paragraph(textNode('{{/if}}')),
169+
paragraph(textNode('After block')),
170+
];
171+
const result = processContentArray(nodes, vars, { hipaa: false, soc2: true });
172+
expect(result).toHaveLength(1);
173+
expect((result[0] as any).content[0].text).toBe('After block');
174+
});
175+
176+
it('deeply nested: outer false hides all inner levels', () => {
177+
const nodes = [
178+
paragraph(textNode('{{#if hipaa}}')),
179+
paragraph(textNode('{{#if soc2}}')),
180+
paragraph(textNode('{{#if gdpr}}')),
181+
paragraph(textNode('Deep content')),
182+
paragraph(textNode('{{/if}}')),
183+
paragraph(textNode('{{/if}}')),
184+
paragraph(textNode('{{/if}}')),
185+
];
186+
const result = processContentArray(nodes, vars, { hipaa: false, soc2: true, gdpr: true });
187+
expect(result).toHaveLength(0);
188+
});
189+
});
190+
191+
describe('placeholder + conditional combined', () => {
192+
it('replaces placeholders inside kept conditional blocks', () => {
193+
const nodes = [
194+
paragraph(textNode('{{#if soc2}}')),
195+
paragraph(textNode('{{COMPANY}} complies with SOC 2')),
196+
paragraph(textNode('{{/if}}')),
197+
];
198+
const result = processContentArray(nodes, vars, { soc2: true });
199+
expect(result).toHaveLength(1);
200+
expect((result[0] as any).content[0].text).toBe('Acme Inc complies with SOC 2');
201+
});
202+
203+
it('does not process placeholders in removed blocks', () => {
204+
const nodes = [
205+
paragraph(textNode('{{#if hipaa}}')),
206+
paragraph(textNode('{{COMPANY}} handles PHI')),
207+
paragraph(textNode('{{/if}}')),
208+
];
209+
const result = processContentArray(nodes, vars, { hipaa: false });
210+
expect(result).toHaveLength(0);
211+
});
212+
});
213+
214+
describe('edge cases', () => {
215+
it('empty content array returns empty', () => {
216+
expect(processContentArray([], vars, {})).toEqual([]);
217+
});
218+
219+
it('node with no text or content passes through', () => {
220+
const nodes = [{ type: 'hardBreak' }];
221+
const result = processContentArray(nodes, vars, {});
222+
expect(result).toHaveLength(1);
223+
expect(result[0]).toEqual({ type: 'hardBreak' });
224+
});
225+
226+
it('removes empty text nodes after placeholder replacement', () => {
227+
const nodes = [paragraph(textNode('{{#if hipaa}}{{/if}}'))];
228+
const result = processContentArray(nodes, vars, { hipaa: false });
229+
// Inline conditional removes content, leaving empty string → null → paragraph has no content
230+
expect(result).toHaveLength(0);
231+
});
232+
233+
it('preserves node attributes and marks', () => {
234+
const nodes = [{
235+
type: 'paragraph',
236+
attrs: { textAlign: 'center' },
237+
content: [{
238+
type: 'text',
239+
text: '{{COMPANY}} policy',
240+
marks: [{ type: 'bold' }],
241+
}],
242+
}];
243+
const result = processContentArray(nodes, vars, {});
244+
const node = result[0] as any;
245+
expect(node.attrs.textAlign).toBe('center');
246+
expect(node.content[0].text).toBe('Acme Inc policy');
247+
expect(node.content[0].marks).toEqual([{ type: 'bold' }]);
248+
});
249+
});
250+
});
251+
252+
describe('buildVariables', () => {
253+
it('maps COMPANY from companyName', () => {
254+
const vars = buildVariables({ companyName: 'TestCo', contextHub: '' });
255+
expect(vars.COMPANY).toBe('TestCo');
256+
});
257+
258+
it('extracts answers from contextHub Q&A format', () => {
259+
const contextHub = 'What industry is your company in?\nSaaS\nHow many employees do you have?\n50';
260+
const vars = buildVariables({ companyName: 'X', contextHub });
261+
expect(vars.INDUSTRY).toBe('SaaS');
262+
expect(vars.EMPLOYEES).toBe('50');
263+
});
264+
265+
it('handles missing questions gracefully', () => {
266+
const vars = buildVariables({ companyName: 'X', contextHub: 'Random text' });
267+
expect(vars.INDUSTRY).toBeUndefined();
268+
});
269+
});
270+
271+
describe('buildFlags', () => {
272+
it('detects SOC 2 framework', () => {
273+
const flags = buildFlags([{ name: 'SOC 2' }]);
274+
expect(flags.soc2).toBe(true);
275+
expect(flags.hipaa).toBe(false);
276+
});
277+
278+
it('detects multiple frameworks', () => {
279+
const flags = buildFlags([{ name: 'SOC 2' }, { name: 'HIPAA' }, { name: 'GDPR' }]);
280+
expect(flags.soc2).toBe(true);
281+
expect(flags.hipaa).toBe(true);
282+
expect(flags.gdpr).toBe(true);
283+
expect(flags.pipeda).toBe(false);
284+
});
285+
286+
it('detects PIPEDA', () => {
287+
const flags = buildFlags([{ name: 'PIPEDA' }]);
288+
expect(flags.pipeda).toBe(true);
289+
});
290+
});
291+
292+
describe('processTemplate', () => {
293+
it('handles doc-wrapped content', () => {
294+
const content = {
295+
type: 'doc',
296+
content: [paragraph(textNode('{{COMPANY}} policy'))],
297+
};
298+
const result = processTemplate({
299+
content,
300+
companyName: 'TestCo',
301+
contextHub: '',
302+
frameworks: [],
303+
});
304+
expect(result).toHaveLength(1);
305+
expect((result[0] as any).content[0].text).toBe('TestCo policy');
306+
});
307+
308+
it('handles array content', () => {
309+
const content = [paragraph(textNode('Hello {{COMPANY}}'))];
310+
const result = processTemplate({
311+
content,
312+
companyName: 'TestCo',
313+
contextHub: '',
314+
frameworks: [],
315+
});
316+
expect((result[0] as any).content[0].text).toBe('Hello TestCo');
317+
});
318+
319+
it('returns empty for invalid content', () => {
320+
expect(processTemplate({ content: null, companyName: '', contextHub: '', frameworks: [] })).toEqual([]);
321+
expect(processTemplate({ content: 'string', companyName: '', contextHub: '', frameworks: [] })).toEqual([]);
322+
expect(processTemplate({ content: 42, companyName: '', contextHub: '', frameworks: [] })).toEqual([]);
323+
});
324+
});

0 commit comments

Comments
 (0)