feat(platform): add PII policy tab to governance page#1370
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis PR implements a dedicated PII (Personally Identifiable Information) policy configuration interface in the governance settings dashboard. It adds a new "pii" tab to the governance settings page with structured configuration for PII detection and masking, introduces Zod schema validation for PII configurations ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/convex/governance/mutations.ts`:
- Around line 127-135: upsertPolicy currently accepts policyType: 'pii_config'
with config: v.any() and bypasses the schema check you added in upsertPiiConfig;
update upsertPolicy to detect when args.policyType === 'pii_config' and validate
args.config using piiConfigSchema.safeParse (same validation logic as in
upsertPiiConfig), throwing a descriptive error if parsed.success is false before
persisting; reference the upsertPolicy function, the piiConfigSchema, and the
policyType/config fields when making the change.
In `@services/platform/convex/governance/pii/__tests__/pii_patterns.test.ts`:
- Around line 124-126: The test uses `as const` to narrow test config objects
(e.g., the `config` variable) which over-constrains the `mode` field; replace
those `as const` assertions with `satisfies PiiConfig` so the object shape is
validated against the `PiiConfig` type without forcing literal narrowing (update
occurrences for `config` and any other test config objects in this file, keeping
the object values the same but changing the assertion to `satisfies PiiConfig`).
In `@services/platform/convex/governance/pii/pii_patterns.ts`:
- Around line 53-57: The current germanId pattern allows a digit as the first
character; update the regex for the object with name 'germanId' so the first
character is restricted to the allowed letters only (e.g. change the leading
character class from [CFGHJKLMNPRTVWXYZ\d] to [CFGHJKLMNPRTVWXYZ] and keep the
following {8} characters as the existing allowed set), and add a unit test
asserting that a numeric-only string like "123456789" does NOT match or get
masked to prevent regressions; reference the 'germanId' entry and the test file
that covers pii_patterns matching when making the change.
In `@services/platform/lib/shared/schemas/governance.ts`:
- Around line 78-81: piiCustomPatternSchema currently allows any non-empty
string for regex which can lead to runtime RegExp errors in buildPatterns() when
calling new RegExp(cp.regex, 'g'); update piiCustomPatternSchema to validate
regex syntax by adding a refinement on the regex field that attempts to
construct a RegExp (e.g., try new RegExp(value) and return true on success,
false on throw) and return a clear error message like "invalid regex pattern";
reference piiCustomPatternSchema and the consumer buildPatterns()/new
RegExp(cp.regex, 'g') so reviewers can locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b07c8f9-37ef-432b-a082-4692d1baea91
📒 Files selected for processing (8)
services/platform/app/routes/dashboard/$id/settings/governance.tsxservices/platform/convex/governance/mutations.tsservices/platform/convex/governance/pii/__tests__/pii_patterns.test.tsservices/platform/convex/governance/pii/pii_patterns.tsservices/platform/lib/shared/schemas/governance.tsservices/platform/messages/de-CH.jsonservices/platform/messages/de.jsonservices/platform/messages/en.json
| const parsed = piiConfigSchema.safeParse({ | ||
| enabled: args.enabled, | ||
| mode: args.mode, | ||
| enabledPatterns: args.enabledPatterns, | ||
| customPatterns: args.customPatterns, | ||
| }); | ||
| if (!parsed.success) { | ||
| throw new Error(`Invalid PII configuration: ${parsed.error.message}`); | ||
| } |
There was a problem hiding this comment.
Close the pii_config validation bypass in generic upsert.
Line 127 adds validation in upsertPiiConfig, but upsertPolicy (Line 18 onward) still allows policyType: 'pii_config' with config: v.any() and no schema check. That leaves a direct path to persist invalid PII policy config.
🔧 Suggested fix
if (args.policyType === 'budgets') {
const parsed = budgetConfigSchema.safeParse(args.config);
if (!parsed.success) {
throw new Error(
`Invalid budget configuration: ${parsed.error.message}`,
);
}
}
+ if (args.policyType === 'pii_config') {
+ const parsed = piiConfigSchema.safeParse(args.config);
+ if (!parsed.success) {
+ throw new Error(`Invalid PII configuration: ${parsed.error.message}`);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const parsed = piiConfigSchema.safeParse({ | |
| enabled: args.enabled, | |
| mode: args.mode, | |
| enabledPatterns: args.enabledPatterns, | |
| customPatterns: args.customPatterns, | |
| }); | |
| if (!parsed.success) { | |
| throw new Error(`Invalid PII configuration: ${parsed.error.message}`); | |
| } | |
| if (args.policyType === 'budgets') { | |
| const parsed = budgetConfigSchema.safeParse(args.config); | |
| if (!parsed.success) { | |
| throw new Error( | |
| `Invalid budget configuration: ${parsed.error.message}`, | |
| ); | |
| } | |
| } | |
| if (args.policyType === 'pii_config') { | |
| const parsed = piiConfigSchema.safeParse(args.config); | |
| if (!parsed.success) { | |
| throw new Error(`Invalid PII configuration: ${parsed.error.message}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/governance/mutations.ts` around lines 127 - 135,
upsertPolicy currently accepts policyType: 'pii_config' with config: v.any() and
bypasses the schema check you added in upsertPiiConfig; update upsertPolicy to
detect when args.policyType === 'pii_config' and validate args.config using
piiConfigSchema.safeParse (same validation logic as in upsertPiiConfig),
throwing a descriptive error if parsed.success is false before persisting;
reference the upsertPolicy function, the piiConfigSchema, and the
policyType/config fields when making the change.
| const config = { | ||
| enabled: true, | ||
| mode: 'mask' as const, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "pii_patterns.test.ts" | head -20Repository: tale-project/tale
Length of output: 133
🏁 Script executed:
if [ -f "services/platform/convex/governance/pii/__tests__/pii_patterns.test.ts" ]; then
wc -l "services/platform/convex/governance/pii/__tests__/pii_patterns.test.ts"
fiRepository: tale-project/tale
Length of output: 135
🏁 Script executed:
if [ -f "services/platform/convex/governance/pii/__tests__/pii_patterns.test.ts" ]; then
sed -n '120,160p' "services/platform/convex/governance/pii/__tests__/pii_patterns.test.ts"
fiRepository: tale-project/tale
Length of output: 1416
🏁 Script executed:
# Find the scrubPii function definition
fd . -type f -name "*.ts" | xargs rg -l "export.*scrubPii|function scrubPii" | head -10Repository: tale-project/tale
Length of output: 297
🏁 Script executed:
sed -n '1,100p' "services/platform/convex/governance/pii/index.ts"Repository: tale-project/tale
Length of output: 1878
🏁 Script executed:
# Get the full scrubPii function signature and parameters
rg -A 15 "function scrubPii|export.*scrubPii|const scrubPii" "services/platform/convex/governance/pii/index.ts"Repository: tale-project/tale
Length of output: 552
Replace as const assertions with satisfies PiiConfig for test config objects.
The as const assertions here narrow the mode field to literal types ('mask', 'block'), which is stricter than the PiiConfig interface requires. Since the interface expects mode: 'mask' | 'block' and the code doesn't rely on literal type narrowing, these assertions are unnecessary. Use satisfies instead to validate the object structure without over-narrowing.
🔧 Suggested refactor
const config = {
enabled: true,
- mode: 'mask' as const,
+ mode: 'mask',
enabledPatterns: ['iban', 'germanId', 'email'],
customPatterns: [],
-};
+} satisfies PiiConfig;
-const blockConfig = { ...config, mode: 'block' as const };
+const blockConfig = {
+ ...config,
+ mode: 'block',
+} satisfies PiiConfig;Also applies to: lines 145–152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/governance/pii/__tests__/pii_patterns.test.ts`
around lines 124 - 126, The test uses `as const` to narrow test config objects
(e.g., the `config` variable) which over-constrains the `mode` field; replace
those `as const` assertions with `satisfies PiiConfig` so the object shape is
validated against the `PiiConfig` type without forcing literal narrowing (update
occurrences for `config` and any other test config objects in this file, keeping
the object values the same but changing the assertion to `satisfies PiiConfig`).
| { | ||
| name: 'germanId', | ||
| regex: /\b[CFGHJKLMNPRTVWXYZ\d][CFGHJKLMNPRTVWXYZ\d]{8}\b/g, | ||
| replacement: '[GERMAN_ID]', | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current pattern and test coverage for numeric-only false positives.
sed -n '50,60p' services/platform/convex/governance/pii/pii_patterns.ts
rg -n "germanId|123456789|does not match" services/platform/convex/governance/pii/__tests__/pii_patterns.test.tsRepository: tale-project/tale
Length of output: 1094
🏁 Script executed:
# Search for all test cases in the germanId test section
rg -A 3 "germanId" services/platform/convex/governance/pii/__tests__/pii_patterns.test.ts | head -80
# Check if there are tests for numeric-only strings
rg "123456789|^\d{9}" services/platform/convex/governance/pii/__tests__/pii_patterns.test.tsRepository: tale-project/tale
Length of output: 1267
🏁 Script executed:
# Get the complete germanId test section to see all test cases
sed -n '93,120p' services/platform/convex/governance/pii/__tests__/pii_patterns.test.ts
# Check if any integration or other test files test germanId
fd -e test.ts -e test.js | xargs rg "germanId" 2>/dev/null | wc -lRepository: tale-project/tale
Length of output: 1007
🏁 Script executed:
# Verify the actual regex behavior with numeric-only strings
python3 << 'EOF'
import re
# Current regex from the code
current_pattern = r'\b[CFGHJKLMNPRTVWXYZ\d][CFGHJKLMNPRTVWXYZ\d]{8}\b'
# Suggested fix
suggested_pattern = r'\b[CFGHJKLMNPRTVWXYZ][CFGHJKLMNPRTVWXYZ\d]{8}\b'
current_regex = re.compile(current_pattern, re.MULTILINE)
suggested_regex = re.compile(suggested_pattern, re.MULTILINE)
test_cases = [
('123456789', 'all-digit 9-char'),
('T22000129', 'letter+digits'),
('L0100FG57', 'letter+mixed'),
('1T2000012', 'digit+letter+digits'),
]
print("Current regex behavior:")
for test, desc in test_cases:
match = current_regex.search(test)
print(f" {desc:25} '{test}' -> {'MATCH' if match else 'NO MATCH'}")
print("\nSuggested regex behavior:")
for test, desc in test_cases:
match = suggested_regex.search(test)
print(f" {desc:25} '{test}' -> {'MATCH' if match else 'NO MATCH'}")
EOFRepository: tale-project/tale
Length of output: 509
germanId regex is too permissive and matches invalid identifiers.
Current pattern allows any 9-character string starting with a digit (e.g., 123456789), which can incorrectly trigger masking in normal messages. German Personalausweis IDs must start with a valid letter from the allowed character set.
🔧 Suggested fix
- regex: /\b[CFGHJKLMNPRTVWXYZ\d][CFGHJKLMNPRTVWXYZ\d]{8}\b/g,
+ regex: /\b[CFGHJKLMNPRTVWXYZ][CFGHJKLMNPRTVWXYZ\d]{8}\b/g,Additionally, add a test case to prevent regression of numeric-only false positives.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| name: 'germanId', | |
| regex: /\b[CFGHJKLMNPRTVWXYZ\d][CFGHJKLMNPRTVWXYZ\d]{8}\b/g, | |
| replacement: '[GERMAN_ID]', | |
| }, | |
| { | |
| name: 'germanId', | |
| regex: /\b[CFGHJKLMNPRTVWXYZ][CFGHJKLMNPRTVWXYZ\d]{8}\b/g, | |
| replacement: '[GERMAN_ID]', | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/convex/governance/pii/pii_patterns.ts` around lines 53 -
57, The current germanId pattern allows a digit as the first character; update
the regex for the object with name 'germanId' so the first character is
restricted to the allowed letters only (e.g. change the leading character class
from [CFGHJKLMNPRTVWXYZ\d] to [CFGHJKLMNPRTVWXYZ] and keep the following {8}
characters as the existing allowed set), and add a unit test asserting that a
numeric-only string like "123456789" does NOT match or get masked to prevent
regressions; reference the 'germanId' entry and the test file that covers
pii_patterns matching when making the change.
| export const piiCustomPatternSchema = z.object({ | ||
| name: z.string().min(1), | ||
| regex: z.string().min(1), | ||
| replacement: z.string().min(1), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect where custom regex is compiled and whether compilation is guarded.
rg -n "new RegExp\\(|RegExp\\(" services/platform/convex/governance/pii
rg -n "try\\s*\\{" services/platform/convex/governance/piiRepository: tale-project/tale
Length of output: 282
🏁 Script executed:
cat -n services/platform/lib/shared/schemas/governance.ts | sed -n '75,95p'Repository: tale-project/tale
Length of output: 741
🏁 Script executed:
cat -n services/platform/convex/governance/pii/pii_detector.ts | sed -n '10,25p'Repository: tale-project/tale
Length of output: 696
🏁 Script executed:
cat -n services/platform/convex/governance/pii/index.ts | sed -n '25,40p'Repository: tale-project/tale
Length of output: 658
🏁 Script executed:
rg -B 5 -A 10 "buildPatterns\(" services/platform/convex/governance/pii/index.tsRepository: tale-project/tale
Length of output: 954
🏁 Script executed:
grep -r "scrubPii\|buildPatterns" services/platform/convex/governance/pii --include="*.ts" -nRepository: tale-project/tale
Length of output: 1362
🏁 Script executed:
cat -n services/platform/convex/governance/pii/__tests__/pii_patterns.test.ts | head -170Repository: tale-project/tale
Length of output: 6598
🏁 Script executed:
rg -n "piiConfigSchema\|PiiConfig" services/platform/convex/governance/ --include="*.ts" -B 2 -A 2Repository: tale-project/tale
Length of output: 502
🏁 Script executed:
rg -n "customPatterns" services/platform/convex/governance/ --include="*.ts" | head -20Repository: tale-project/tale
Length of output: 502
🏁 Script executed:
rg -n "piiConfigSchema|PiiConfig" services/platform/convex/governance/ -t ts -B 2 -A 2Repository: tale-project/tale
Length of output: 2795
🏁 Script executed:
rg -n "customPatterns" services/platform/convex/governance/ -t ts | head -20Repository: tale-project/tale
Length of output: 643
🏁 Script executed:
cat -n services/platform/convex/governance/mutations.ts | sed -n '97,155p'Repository: tale-project/tale
Length of output: 2115
Add regex syntax validation to piiCustomPatternSchema.
Line 80 accepts any non-empty string for the regex field. Invalid regex patterns pass schema validation but cause runtime errors when new RegExp(cp.regex, 'g') is called in buildPatterns() at index.ts:31. Unlike email validation (where SSO providers validate upstream), custom regex patterns receive no prior validation.
🔧 Suggested fix
+const isValidRegex = (pattern: string): boolean => {
+ try {
+ new RegExp(pattern);
+ return true;
+ } catch {
+ return false;
+ }
+};
+
export const piiCustomPatternSchema = z.object({
name: z.string().min(1),
- regex: z.string().min(1),
+ regex: z.string().min(1).refine(isValidRegex, 'Invalid regex pattern'),
replacement: z.string().min(1),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const piiCustomPatternSchema = z.object({ | |
| name: z.string().min(1), | |
| regex: z.string().min(1), | |
| replacement: z.string().min(1), | |
| const isValidRegex = (pattern: string): boolean => { | |
| try { | |
| new RegExp(pattern); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| }; | |
| export const piiCustomPatternSchema = z.object({ | |
| name: z.string().min(1), | |
| regex: z.string().min(1).refine(isValidRegex, 'Invalid regex pattern'), | |
| replacement: z.string().min(1), |
🧰 Tools
🪛 GitHub Actions: Lint
[warning] 78-78: knip: Unused export 'piiCustomPatternSchema' (Unused exports: 1).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/lib/shared/schemas/governance.ts` around lines 78 - 81,
piiCustomPatternSchema currently allows any non-empty string for regex which can
lead to runtime RegExp errors in buildPatterns() when calling new
RegExp(cp.regex, 'g'); update piiCustomPatternSchema to validate regex syntax by
adding a refinement on the regex field that attempts to construct a RegExp
(e.g., try new RegExp(value) and return true on success, false on throw) and
return a clear error message like "invalid regex pattern"; reference
piiCustomPatternSchema and the consumer buildPatterns()/new RegExp(cp.regex,
'g') so reviewers can locate the change.
yannickmonney
force-pushed
the
feat/issue-1345-pii-policy-ui
branch
from
4c81795 to
bbd219a
Compare
Wire existing PiiConfig component into governance settings as a fourth tab. Migrate hardcoded tab labels to i18n keys. Fix translation key mismatch between component and locale files. Add IBAN and German ID detection patterns. Add shared Zod schema for PII config validation. - Add PII tab with translated label alongside System Prompt, Budgets, Usage tabs - Restructure governance.pii translation keys to match component expectations (nested patterns, descriptive labels) - Add governance.tabs i18n keys for all tab labels (en, de, de-CH) - Add German translations for all governance subsections - Add IBAN and German ID (Personalausweis) regex patterns to pii_patterns.ts - Add piiConfigSchema and piiCustomPatternSchema to shared governance schemas - Sync POLICY_TYPES to include system_prompt and pii_config - Add Zod validation in upsertPiiConfig mutation - Add backend tests for new patterns and scrubPii integration Closes #1345
- Add pii_config validation in upsertPolicy mutation to prevent schema bypass - Fix germanId regex to reject digit-only strings (e.g. 123456789) - Replace `as const` with `satisfies PiiConfig` in test config objects - Remove unused piiCustomPatternSchema export and add regex validation
larryro
force-pushed
the
feat/issue-1345-pii-policy-ui
branch
from
bbd219a to
53823f7
Compare
Track server state after load and save, compare against current form values to compute isDirty flag for the Save changes button.
…terns - Toggle, mode, and pattern changes now save immediately - Custom pattern cards use Save + Cancel instead of Delete - Test detection requires explicit Test button click - Remove redundant bottom Save changes button
Add client-side PII validation before creating a chat thread so that blocked messages never produce empty threads. Also improve the error toast to show the specific PII block reason instead of a generic "Failed to send message".
2 participants
Summary
PiiConfigcomponent into governance settings page as a fourth tab (after System Prompt, Budgets, Usage)governance.tabs.*)PiiConfigcomponent and locale filespii_patterns.tspiiConfigSchema) for PII config validationPOLICY_TYPESto includesystem_promptandpii_configupsertPiiConfigmutation before writing to DBde-CH) overrides for ss/sz differencesChanges
governance.tsxpii_patterns.tsmutations.tsgovernance.ts(schemas)en.jsonde.jsonde-CH.jsonpii_patterns.test.tsTesting checklist
Closes #1345
Summary by CodeRabbit
New Features
Documentation