mig: fold legacy risk policy scope into per-category detection scopes - #6096
mig: fold legacy risk policy scope into per-category detection scopes#6096vishalg0wda wants to merge 3 commits into
Conversation
Risk policies carry two scoping surfaces that the scanner intersects: the legacy policy-level message_types/scope_include/scope_exempt, and per-category detection scopes. The editor stopped editing the legacy one, so policies narrowed by it could not be widened from the dashboard. This adds the offline fold that leaves detection scopes as the only surface. Because both surfaces narrow, dropping the legacy one widens what a policy scans, so the fold is conditional on the policy action: enforcing policies (warn, block, quarantine) get the legacy scope composed into each category scope and keep scanning identically, while flag policies drop it and fall back to the recommendation registry. Claude-Session: https://claude.ai/code/session_01BDoR4rSHZNpXdSk1XA7AsJ
|
|
Running ultrareview automatically — This data migration rewrites risk-policy scopes in production; a fold-logic bug could silently widen or narrow enforcement for warn/block/quarantine policies, corrupting scan behavior.. I'll post findings when complete. |
policycatalog.PolicyMessageTypes omits prompt_attachment by design, but UpdateRiskPolicy validated message_types against message.IsTypeValid, so stored rows can carry it. Encoding the legacy kind allowlist against the authoring catalog would abort the run on valid data. Claude-Session: https://claude.ai/code/session_01BDoR4rSHZNpXdSk1XA7AsJ
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultrareview completed in 8m 33s
11 issues found and verified against the latest diff
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="server/cmd/tools/migrations/legacypolicyscope/queries.sql">
<violation number="1" location="server/cmd/tools/migrations/legacypolicyscope/queries.sql:27">
P2: When a matching policy is locked, `SKIP LOCKED` drops it from the result. Dry-run then advances past its ID, and apply can finish successfully with `remaining > 0`; retry from the beginning until the remaining count is zero, or use a separate non-locking dry-run path.</violation>
<violation number="2" location="server/cmd/tools/migrations/legacypolicyscope/queries.sql:41">
P1: Custom agent: **Flag Security Vulnerabilities**
These new SQLc queries have no tenant boundary: the apply path clears a policy using only its ID, and the batch/count paths likewise omit `project_id` and `organization_id`. Because this command has no tenant selector, an apply run locks and mutates matching policies across organizations. Add a validated organization or project parameter to every batch, update, and count predicate, and carry it through the SQLc callers.</violation>
<violation number="3" location="server/cmd/tools/migrations/legacypolicyscope/queries.sql:45">
P2: `CountRemainingLegacyScopes` runs outside the transaction that sets `statement_timeout`, so dry-run and validate can scan a large `risk_policies` table without the configured timeout. Execute this count in a timeout-configured transaction, as the batch does, or apply an explicit context deadline.</violation>
</file>
<file name="server/cmd/tools/migrations/legacy_policy_scope_cmd.go">
<violation number="1" location="server/cmd/tools/migrations/legacy_policy_scope_cmd.go:60">
P2: `-batch-size` accepts values above the `int32` SQL parameter used by `LockLegacyScopeBatch`. Casting `2147483648` wraps negative and makes PostgreSQL reject the first `LIMIT`; cap it at `math.MaxInt32`.</violation>
<violation number="2" location="server/cmd/tools/migrations/legacy_policy_scope_cmd.go:60">
P2: Positive sub-millisecond timeout flags pass this check, but `durationSetting` serializes them as `0ms`, which disables PostgreSQL’s timeout. Require both timeouts to be at least one millisecond.</violation>
<violation number="3" location="server/cmd/tools/migrations/legacy_policy_scope_cmd.go:66">
P1: When `-environment` contains surrounding whitespace or different casing, this check skips the required production confirmation. Trim and normalize the environment before this comparison.</violation>
<violation number="4" location="server/cmd/tools/migrations/legacy_policy_scope_cmd.go:97">
P1: Custom agent: **Flag Security Vulnerabilities**
`pgxpool.New` accepts `GRAM_DATABASE_URL` without requiring TLS. A URL with `sslmode=disable`, or pgx's plaintext fallback mode, can send policy data and database credentials unencrypted. Parse the URL and reject disabled or fallback-to-plaintext configurations before creating the pool.
(Based on your team's feedback about requiring TLS for `GRAM_DATABASE_URL`.)</violation>
</file>
<file name="server/cmd/tools/migrations/legacypolicyscope/fold.go">
<violation number="1" location="server/cmd/tools/migrations/legacypolicyscope/fold.go:136">
P1: Custom agent: **Flag Security Vulnerabilities**
According to linked Linear issue AIS-678, enforcing policies must preserve the legacy prefilter. When `risk-recommended-scopes` is disabled (the documented default), this preserved result clears the legacy columns while both scanners ignore the replacement detection scopes, so a matching secret in a previously excluded message can trigger block, warn, or quarantine. Gate clearing until the flag is enabled or make migrated explicit scopes apply independently of that flag.</violation>
</file>
<file name="server/cmd/tools/migrations/legacypolicyscope/queries.sql.go">
<violation number="1" location="server/cmd/tools/migrations/legacypolicyscope/queries.sql.go:51">
P2: According to linked Linear issue AIS-678, the fold must preserve existing policy behavior. Whitespace-only `scope_include` and `scope_exempt` values are valid no-ops at scan time, but these predicates treat them as active legacy scopes and can make the fold fail CEL compilation. Normalize or ignore whitespace-only scope values before selecting and composing rows.</violation>
</file>
<file name="server/cmd/tools/migrations/LEGACY_POLICY_SCOPE_MIGRATION.md">
<violation number="1" location="server/cmd/tools/migrations/LEGACY_POLICY_SCOPE_MIGRATION.md:69">
P2: According to linked Linear issue AIS-678, onboarding still writes the legacy scope, and Platform MCP can also write it. Add an explicit prerequisite to deploy the writer changes or quiesce all legacy writers before applying; otherwise new policies can be narrowed again immediately after this migration.</violation>
<violation number="2" location="server/cmd/tools/migrations/LEGACY_POLICY_SCOPE_MIGRATION.md:123">
P2: The rollback backup omits `analyzer_config`, even though the fold rewrites it and drops unrecognized keys. Include `analyzer_config` in the backup and restore it during rollback so a failed or reverted migration cannot lose scanner configuration.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| @@ -0,0 +1,64 @@ | |||
| -- name: SetLocalTimeouts :one | |||
There was a problem hiding this comment.
P1: Custom agent: Flag Security Vulnerabilities
These new SQLc queries have no tenant boundary: the apply path clears a policy using only its ID, and the batch/count paths likewise omit project_id and organization_id. Because this command has no tenant selector, an apply run locks and mutates matching policies across organizations. Add a validated organization or project parameter to every batch, update, and count predicate, and carry it through the SQLc callers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/cmd/tools/migrations/legacypolicyscope/queries.sql, line 41:
<comment>These new SQLc queries have no tenant boundary: the apply path clears a policy using only its ID, and the batch/count paths likewise omit `project_id` and `organization_id`. Because this command has no tenant selector, an apply run locks and mutates matching policies across organizations. Add a validated organization or project parameter to every batch, update, and count predicate, and carry it through the SQLc callers.</comment>
<file context>
@@ -0,0 +1,64 @@
+ scope_exempt = NULL,
+ version = CASE WHEN @bump_version::boolean THEN version + 1 ELSE version END,
+ updated_at = clock_timestamp()
+WHERE id = @id::uuid
+ AND deleted IS FALSE;
+
</file context>
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| pool, err := pgxpool.New(ctx, cfg.dbURL) |
There was a problem hiding this comment.
P1: Custom agent: Flag Security Vulnerabilities
pgxpool.New accepts GRAM_DATABASE_URL without requiring TLS. A URL with sslmode=disable, or pgx's plaintext fallback mode, can send policy data and database credentials unencrypted. Parse the URL and reject disabled or fallback-to-plaintext configurations before creating the pool.
(Based on your team's feedback about requiring TLS for GRAM_DATABASE_URL.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/cmd/tools/migrations/legacy_policy_scope_cmd.go, line 97:
<comment>`pgxpool.New` accepts `GRAM_DATABASE_URL` without requiring TLS. A URL with `sslmode=disable`, or pgx's plaintext fallback mode, can send policy data and database credentials unencrypted. Parse the URL and reject disabled or fallback-to-plaintext configurations before creating the pool.
(Based on your team's feedback about requiring TLS for `GRAM_DATABASE_URL`.) </comment>
<file context>
@@ -0,0 +1,147 @@
+ ctx, cancel := context.WithCancel(context.Background())
+ defer cancel()
+
+ pool, err := pgxpool.New(ctx, cfg.dbURL)
+ if err != nil {
+ log.Printf("connect postgres for legacy-policy-scope failed")
</file context>
| } | ||
| return 0 | ||
| }) | ||
| return Result{Disposition: DispositionPreserved, DetectionScopes: scopes}, nil |
There was a problem hiding this comment.
P1: Custom agent: Flag Security Vulnerabilities
According to linked Linear issue AIS-678, enforcing policies must preserve the legacy prefilter. When risk-recommended-scopes is disabled (the documented default), this preserved result clears the legacy columns while both scanners ignore the replacement detection scopes, so a matching secret in a previously excluded message can trigger block, warn, or quarantine. Gate clearing until the flag is enabled or make migrated explicit scopes apply independently of that flag.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/cmd/tools/migrations/legacypolicyscope/fold.go, line 135:
<comment>According to linked Linear issue AIS-678, enforcing policies must preserve the legacy prefilter. When `risk-recommended-scopes` is disabled (the documented default), this preserved result clears the legacy columns while both scanners ignore the replacement detection scopes, so a matching secret in a previously excluded message can trigger block, warn, or quarantine. Gate clearing until the flag is enabled or make migrated explicit scopes apply independently of that flag.</comment>
<file context>
@@ -0,0 +1,204 @@
+ }
+ return 0
+ })
+ return Result{Disposition: DispositionPreserved, DetectionScopes: scopes}, nil
+}
+
</file context>
| if *apply && *confirmEnvironment != *environment { | ||
| return legacyPolicyScopeConfig{}, errors.New("writes require -confirm-environment to exactly match -environment") | ||
| } | ||
| if *apply && *environment == "production" && *confirmProduction != "production" { |
There was a problem hiding this comment.
P1: When -environment contains surrounding whitespace or different casing, this check skips the required production confirmation. Trim and normalize the environment before this comparison.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/cmd/tools/migrations/legacy_policy_scope_cmd.go, line 66:
<comment>When `-environment` contains surrounding whitespace or different casing, this check skips the required production confirmation. Trim and normalize the environment before this comparison.</comment>
<file context>
@@ -0,0 +1,147 @@
+ if *apply && *confirmEnvironment != *environment {
+ return legacyPolicyScopeConfig{}, errors.New("writes require -confirm-environment to exactly match -environment")
+ }
+ if *apply && *environment == "production" && *confirmProduction != "production" {
+ return legacyPolicyScopeConfig{}, errors.New("production writes require -confirm-production=production")
+ }
</file context>
| if *apply && *environment == "production" && *confirmProduction != "production" { | |
| if *apply && strings.EqualFold(strings.TrimSpace(*environment), "production") && *confirmProduction != "production" { |
| AND deleted IS FALSE; | ||
|
|
||
| -- name: CountRemainingLegacyScopes :one | ||
| SELECT count(*) AS remaining |
There was a problem hiding this comment.
P2: CountRemainingLegacyScopes runs outside the transaction that sets statement_timeout, so dry-run and validate can scan a large risk_policies table without the configured timeout. Execute this count in a timeout-configured transaction, as the batch does, or apply an explicit context deadline.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/cmd/tools/migrations/legacypolicyscope/queries.sql, line 45:
<comment>`CountRemainingLegacyScopes` runs outside the transaction that sets `statement_timeout`, so dry-run and validate can scan a large `risk_policies` table without the configured timeout. Execute this count in a timeout-configured transaction, as the batch does, or apply an explicit context deadline.</comment>
<file context>
@@ -0,0 +1,64 @@
+ AND deleted IS FALSE;
+
+-- name: CountRemainingLegacyScopes :one
+SELECT count(*) AS remaining
+FROM risk_policies AS p
+WHERE p.deleted IS FALSE
</file context>
| if *batchSize <= 0 || *lockTimeout <= 0 || *statementTimeout <= 0 { | ||
| return legacyPolicyScopeConfig{}, errors.New("batch size and timeouts must be positive") |
There was a problem hiding this comment.
P2: -batch-size accepts values above the int32 SQL parameter used by LockLegacyScopeBatch. Casting 2147483648 wraps negative and makes PostgreSQL reject the first LIMIT; cap it at math.MaxInt32.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/cmd/tools/migrations/legacy_policy_scope_cmd.go, line 60:
<comment>`-batch-size` accepts values above the `int32` SQL parameter used by `LockLegacyScopeBatch`. Casting `2147483648` wraps negative and makes PostgreSQL reject the first `LIMIT`; cap it at `math.MaxInt32`.</comment>
<file context>
@@ -0,0 +1,147 @@
+ if strings.TrimSpace(*environment) == "" {
+ return legacyPolicyScopeConfig{}, errors.New("environment is required")
+ }
+ if *batchSize <= 0 || *lockTimeout <= 0 || *statementTimeout <= 0 {
+ return legacyPolicyScopeConfig{}, errors.New("batch size and timeouts must be positive")
+ }
</file context>
| if *batchSize <= 0 || *lockTimeout <= 0 || *statementTimeout <= 0 { | |
| return legacyPolicyScopeConfig{}, errors.New("batch size and timeouts must be positive") | |
| if *batchSize <= 0 || *batchSize > 1<<31-1 || *lockTimeout <= 0 || *statementTimeout <= 0 { | |
| return legacyPolicyScopeConfig{}, errors.New("batch size must be in the int32 range and timeouts must be positive") |
| if *batchSize <= 0 || *lockTimeout <= 0 || *statementTimeout <= 0 { | ||
| return legacyPolicyScopeConfig{}, errors.New("batch size and timeouts must be positive") |
There was a problem hiding this comment.
P2: Positive sub-millisecond timeout flags pass this check, but durationSetting serializes them as 0ms, which disables PostgreSQL’s timeout. Require both timeouts to be at least one millisecond.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/cmd/tools/migrations/legacy_policy_scope_cmd.go, line 60:
<comment>Positive sub-millisecond timeout flags pass this check, but `durationSetting` serializes them as `0ms`, which disables PostgreSQL’s timeout. Require both timeouts to be at least one millisecond.</comment>
<file context>
@@ -0,0 +1,147 @@
+ if strings.TrimSpace(*environment) == "" {
+ return legacyPolicyScopeConfig{}, errors.New("environment is required")
+ }
+ if *batchSize <= 0 || *lockTimeout <= 0 || *statementTimeout <= 0 {
+ return legacyPolicyScopeConfig{}, errors.New("batch size and timeouts must be positive")
+ }
</file context>
| if *batchSize <= 0 || *lockTimeout <= 0 || *statementTimeout <= 0 { | |
| return legacyPolicyScopeConfig{}, errors.New("batch size and timeouts must be positive") | |
| if *batchSize <= 0 || *lockTimeout < time.Millisecond || *statementTimeout < time.Millisecond { | |
| return legacyPolicyScopeConfig{}, errors.New("batch size must be positive and timeouts must be at least 1ms") |
| WHERE p.deleted IS FALSE | ||
| AND ( | ||
| (p.message_types IS NOT NULL AND cardinality(p.message_types) > 0) | ||
| OR coalesce(p.scope_include, '') <> '' |
There was a problem hiding this comment.
P2: According to linked Linear issue AIS-678, the fold must preserve existing policy behavior. Whitespace-only scope_include and scope_exempt values are valid no-ops at scan time, but these predicates treat them as active legacy scopes and can make the fold fail CEL compilation. Normalize or ignore whitespace-only scope values before selecting and composing rows.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/cmd/tools/migrations/legacypolicyscope/queries.sql.go, line 51:
<comment>According to linked Linear issue AIS-678, the fold must preserve existing policy behavior. Whitespace-only `scope_include` and `scope_exempt` values are valid no-ops at scan time, but these predicates treat them as active legacy scopes and can make the fold fail CEL compilation. Normalize or ignore whitespace-only scope values before selecting and composing rows.</comment>
<file context>
@@ -0,0 +1,193 @@
+WHERE p.deleted IS FALSE
+ AND (
+ (p.message_types IS NOT NULL AND cardinality(p.message_types) > 0)
+ OR coalesce(p.scope_include, '') <> ''
+ OR coalesce(p.scope_exempt, '') <> ''
+ )
</file context>
|
|
||
| ```sql | ||
| CREATE TABLE risk_policies_legacy_scope_backup AS | ||
| SELECT id, message_types, scope_include, scope_exempt, version |
There was a problem hiding this comment.
P2: The rollback backup omits analyzer_config, even though the fold rewrites it and drops unrecognized keys. Include analyzer_config in the backup and restore it during rollback so a failed or reverted migration cannot lose scanner configuration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/cmd/tools/migrations/LEGACY_POLICY_SCOPE_MIGRATION.md, line 123:
<comment>The rollback backup omits `analyzer_config`, even though the fold rewrites it and drops unrecognized keys. Include `analyzer_config` in the backup and restore it during rollback so a failed or reverted migration cannot lose scanner configuration.</comment>
<file context>
@@ -0,0 +1,137 @@
+
+```sql
+CREATE TABLE risk_policies_legacy_scope_backup AS
+SELECT id, message_types, scope_include, scope_exempt, version
+FROM risk_policies
+WHERE deleted IS FALSE
</file context>
| SELECT id, message_types, scope_include, scope_exempt, version | |
| SELECT id, message_types, scope_include, scope_exempt, analyzer_config, version |
| # 1. Population check and dry run. Prints per-policy dispositions and a summary. | ||
| go run ./server/cmd/tools/migrations legacy-policy-scope -environment=dev | ||
|
|
||
| # 2. Apply. |
There was a problem hiding this comment.
P2: According to linked Linear issue AIS-678, onboarding still writes the legacy scope, and Platform MCP can also write it. Add an explicit prerequisite to deploy the writer changes or quiesce all legacy writers before applying; otherwise new policies can be narrowed again immediately after this migration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/cmd/tools/migrations/LEGACY_POLICY_SCOPE_MIGRATION.md, line 69:
<comment>According to linked Linear issue AIS-678, onboarding still writes the legacy scope, and Platform MCP can also write it. Add an explicit prerequisite to deploy the writer changes or quiesce all legacy writers before applying; otherwise new policies can be narrowed again immediately after this migration.</comment>
<file context>
@@ -0,0 +1,137 @@
+# 1. Population check and dry run. Prints per-policy dispositions and a summary.
+go run ./server/cmd/tools/migrations legacy-policy-scope -environment=dev
+
+# 2. Apply.
+go run ./server/cmd/tools/migrations legacy-policy-scope \
+ -environment=dev -apply -confirm-environment=dev
</file context>
Both scan paths short-circuit past per-category detection scopes while risk-recommended-scopes is off, which is the rollout default. Folding an enforcing policy in that state moves its narrowing into scopes nothing reads, widening the policy instead of preserving it. Claude-Session: https://claude.ai/code/session_01BDoR4rSHZNpXdSk1XA7AsJ
|
Valid, and it is the important finding on this PR. Confirmed in both paths: Apply now refuses without |
1 participant
Folds the legacy policy-level risk scope into per-category detection scopes so detection scopes become the only scoping surface. Offline tool, no DDL; the column drop is a later contract migration.
Risk policies carry two scoping surfaces and the scanner intersects them (
scanner.go:307-318realtime,analyze_batch.go:233batch). The editor stopped editing the legacy one, so a policy narrowed by it could not be widened from the dashboard.warn/block/quarantineget the legacy scope composed into each category scope and keep scanning identically;flagdrops it and falls back to the recommendation registry.include = legacy AND base,exempt = legacy OR base, where base is the policy's existing specified scope for that category or the registry recommendation. Session-scoped categories are skipped.celenvengine before it is written; a rejected scope would fail the policy closed at scan time, so it aborts the run.versionbumps only on a cleared fold. Findings carryrisk_policy_version, and a preserved fold scans identically, so its findings stay addressable.Dry run by default, keyset-batched with lock timeouts, idempotent. Runbook:
server/cmd/tools/migrations/LEGACY_POLICY_SCOPE_MIGRATION.md.Pairs with #6092, which stops the wizard writing the legacy field and makes the list column read effective scope.
https://linear.app/speakeasy/issue/AIS-678
https://claude.ai/code/session_01BDoR4rSHZNpXdSk1XA7AsJ
Summary by cubic
Folds the legacy policy-level risk scope into per-category detection scopes, making detection scopes the only remaining scoping surface. Enforcing policies keep the same scanning behavior, while
flagpolicies widen after their legacy narrowing is cleared, fixing policies that the dashboard could not widen.-confirm-recommended-scopes-enabledbecause scanners ignore detection scopes whilerisk-recommended-scopesis disabled.prompt_attachmentvalues to fold successfully.Resolves AIS-678.
Written for commit a7c063d. Summary will update on new commits.