Repository navigation
feat(projects): enforce Project membership and retire the connector - #8590
mzxchandra wants to merge 74 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ty-enforcement # Conflicts: # apps/sim/lib/projects/__integration__/foundation.integration.ts
mzxchandra
changed the title
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 150 files
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 150 files
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 150 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
|
@cubic-dev-ai review this PR |
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
6 issues found across 161 files
Confidence score: 3/5
.github/workflows/migrate.ymlcan start the migration before workers have drained; a mutable acknowledgement digest can let the migration removeproject_idwhile workers still use it. Verify the worker drain directly or use a trustworthy acknowledgement.project-repairs.tscan miss pending repair journals whensearch_pathis non-public, allowing cleanup to proceed as if no repairs were pending. Schema-qualify the journal lookup.project-backfill.tstreats PostgreSQL URLs with different sockethostvalues as the same database, which can conflate backfills for separate databases. Include routing parameters in the connection identity.workspace-fixtures.tscan delete a Project that the fixture did not create. Track fixture-created Project IDs and limit cleanup to those.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/db/testing/workspace-fixtures.ts">
<violation number="1" location="packages/db/testing/workspace-fixtures.ts:100">
P2: This cleanup can delete an existing Project, not just one created for the fixture. Track which Projects the fixture created and restrict deletion to those IDs.</violation>
</file>
<file name="packages/db/maintenance/project-repairs.ts">
<violation number="1" location="packages/db/maintenance/project-repairs.ts:30">
P2: The cleanup gate can miss pending repairs when the connection uses a non-public `search_path`: journal creation may target another schema, while this check only looks for the public table. Schema-qualify journal creation, updates, and reads consistently so migration completion cannot bypass pending cleanup.</violation>
</file>
<file name="apps/sim/lib/workspaces/lifecycle.ts">
<violation number="1" location="apps/sim/lib/workspaces/lifecycle.ts:154">
P2: Strict mode treats every `finishWorkflowArchive` rejection as failed provider cleanup, but that function also publishes MCP events after cleanup. A local subscriber exception can therefore leave the repair journal incomplete after provider cleanup succeeded; isolate notification errors or propagate only provider-cleanup failures.</violation>
</file>
<file name="packages/db/maintenance/project-backfill.ts">
<violation number="1" location="packages/db/maintenance/project-backfill.ts:54">
P2: `postgres:///prod?host=/var/run/postgresql` and `postgres:///prod?host=/tmp/other` hash identically because the socket `host` is in the omitted query string. Include PostgreSQL routing parameters in the identity so a manifest cannot be applied to a different socket-backed database with the same name.</violation>
</file>
<file name="apps/sim/lib/projects/backfill-repair.ts">
<violation number="1" location="apps/sim/lib/projects/backfill-repair.ts:89">
P2: Archive repair can leave an existing Project active after its last environment is archived, so the repair completes but migration validation can never pass. Apply the same last-environment Project archival lifecycle used by `archiveWorkspace` before recording the repair complete.</violation>
</file>
<file name=".github/workflows/migrate.yml">
<violation number="1" location=".github/workflows/migrate.yml:87">
P2: This gate does not enforce the required worker drain: it checks only the app ECS tasks and trusts a mutable digest variable for the worker acknowledgement. A prematurely set variable lets the migration remove `project_workspace` while an old Trigger.dev run still accesses it; require a release-scoped worker-drain receipt or an automated worker check before applying enforcement.</violation>
</file>
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
| .where(condition) | ||
| await tx.delete(workspace).where(condition) | ||
| if (rows.length) | ||
| await tx.delete(project).where( |
There was a problem hiding this comment.
P2: This cleanup can delete an existing Project, not just one created for the fixture. Track which Projects the fixture created and restrict deletion to those IDs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/db/testing/workspace-fixtures.ts, line 100:
<comment>This cleanup can delete an existing Project, not just one created for the fixture. Track which Projects the fixture created and restrict deletion to those IDs.</comment>
<file context>
@@ -0,0 +1,112 @@
+ .where(condition)
+ await tx.delete(workspace).where(condition)
+ if (rows.length)
+ await tx.delete(project).where(
+ and(
+ inArray(
</file context>
| await sql`SELECT to_regclass('public.project_backfill_archive_repairs') IS NOT NULL AS present` | ||
| if (!state.present) return 0 | ||
| const [row] = | ||
| await sql`SELECT count(*)::int AS count FROM project_backfill_archive_repairs WHERE completed_at IS NULL` |
There was a problem hiding this comment.
P2: The cleanup gate can miss pending repairs when the connection uses a non-public search_path: journal creation may target another schema, while this check only looks for the public table. Schema-qualify journal creation, updates, and reads consistently so migration completion cannot bypass pending cleanup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/db/maintenance/project-repairs.ts, line 30:
<comment>The cleanup gate can miss pending repairs when the connection uses a non-public `search_path`: journal creation may target another schema, while this check only looks for the public table. Schema-qualify journal creation, updates, and reads consistently so migration completion cannot bypass pending cleanup.</comment>
<file context>
@@ -0,0 +1,32 @@
+ await sql`SELECT to_regclass('public.project_backfill_archive_repairs') IS NOT NULL AS present`
+ if (!state.present) return 0
+ const [row] =
+ await sql`SELECT count(*)::int AS count FROM project_backfill_archive_repairs WHERE completed_at IS NULL`
+ return row.count
+}
</file context>
| requestId, | ||
| strictExternalCleanup: options.strictExternalCleanup, | ||
| }).catch((error: unknown) => { | ||
| if (options.strictExternalCleanup) { |
There was a problem hiding this comment.
P2: Strict mode treats every finishWorkflowArchive rejection as failed provider cleanup, but that function also publishes MCP events after cleanup. A local subscriber exception can therefore leave the repair journal incomplete after provider cleanup succeeded; isolate notification errors or propagate only provider-cleanup failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apps/sim/lib/workspaces/lifecycle.ts, line 154:
<comment>Strict mode treats every `finishWorkflowArchive` rejection as failed provider cleanup, but that function also publishes MCP events after cleanup. A local subscriber exception can therefore leave the repair journal incomplete after provider cleanup succeeded; isolate notification errors or propagate only provider-cleanup failures.</comment>
<file context>
@@ -135,30 +135,44 @@ export async function archiveEnvironmentInTransaction(
+ requestId,
+ strictExternalCleanup: options.strictExternalCleanup,
+ }).catch((error: unknown) => {
+ if (options.strictExternalCleanup) {
+ cleanupErrors.push(error)
+ return
</file context>
| export function projectBackfillDatabaseId(url: string): string { | ||
| const target = new URL(url) | ||
| return createHash('sha256') | ||
| .update(`${target.hostname.toLowerCase()}:${target.port || '5432'}${target.pathname}`) |
There was a problem hiding this comment.
P2: postgres:///prod?host=/var/run/postgresql and postgres:///prod?host=/tmp/other hash identically because the socket host is in the omitted query string. Include PostgreSQL routing parameters in the identity so a manifest cannot be applied to a different socket-backed database with the same name.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/db/maintenance/project-backfill.ts, line 54:
<comment>`postgres:///prod?host=/var/run/postgresql` and `postgres:///prod?host=/tmp/other` hash identically because the socket `host` is in the omitted query string. Include PostgreSQL routing parameters in the identity so a manifest cannot be applied to a different socket-backed database with the same name.</comment>
<file context>
@@ -0,0 +1,444 @@
+export function projectBackfillDatabaseId(url: string): string {
+ const target = new URL(url)
+ return createHash('sha256')
+ .update(`${target.hostname.toLowerCase()}:${target.port || '5432'}${target.pathname}`)
+ .digest('hex')
+}
</file context>
| throw new ProjectBackfillConflict( | ||
| 'Finish the pending repair with its original manifest before replacing it' | ||
| ) | ||
| await archiveEnvironmentInTransaction( |
There was a problem hiding this comment.
P2: Archive repair can leave an existing Project active after its last environment is archived, so the repair completes but migration validation can never pass. Apply the same last-environment Project archival lifecycle used by archiveWorkspace before recording the repair complete.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apps/sim/lib/projects/backfill-repair.ts, line 89:
<comment>Archive repair can leave an existing Project active after its last environment is archived, so the repair completes but migration validation can never pass. Apply the same last-environment Project archival lifecycle used by `archiveWorkspace` before recording the repair complete.</comment>
<file context>
@@ -0,0 +1,117 @@
+ throw new ProjectBackfillConflict(
+ 'Finish the pending repair with its original manifest before replacing it'
+ )
+ await archiveEnvironmentInTransaction(
+ tx,
+ repair.workspaceId,
</file context>
| env: | ||
| ENVIRONMENT: ${{ inputs.environment }} | ||
| EXPECTED_IMAGE_DIGEST: ${{ inputs.environment == 'production' && vars.PROJECT_COLUMN_ENFORCEMENT_READY_IMAGE_DIGEST_PRODUCTION || inputs.environment == 'staging' && vars.PROJECT_COLUMN_ENFORCEMENT_READY_IMAGE_DIGEST_STAGING || '' }} | ||
| run: python3 .github/scripts/check-project-rollout.py --environment "$ENVIRONMENT" --region "$AWS_REGION" --expected-image-digest "$EXPECTED_IMAGE_DIGEST" |
There was a problem hiding this comment.
P2: This gate does not enforce the required worker drain: it checks only the app ECS tasks and trusts a mutable digest variable for the worker acknowledgement. A prematurely set variable lets the migration remove project_workspace while an old Trigger.dev run still accesses it; require a release-scoped worker-drain receipt or an automated worker check before applying enforcement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At .github/workflows/migrate.yml, line 87:
<comment>This gate does not enforce the required worker drain: it checks only the app ECS tasks and trusts a mutable digest variable for the worker acknowledgement. A prematurely set variable lets the migration remove `project_workspace` while an old Trigger.dev run still accesses it; require a release-scoped worker-drain receipt or an automated worker check before applying enforcement.</comment>
<file context>
@@ -51,6 +60,32 @@ jobs:
+ env:
+ ENVIRONMENT: ${{ inputs.environment }}
+ EXPECTED_IMAGE_DIGEST: ${{ inputs.environment == 'production' && vars.PROJECT_COLUMN_ENFORCEMENT_READY_IMAGE_DIGEST_PRODUCTION || inputs.environment == 'staging' && vars.PROJECT_COLUMN_ENFORCEMENT_READY_IMAGE_DIGEST_STAGING || '' }}
+ run: python3 .github/scripts/check-project-rollout.py --environment "$ENVIRONMENT" --region "$AWS_REGION" --expected-image-digest "$EXPECTED_IMAGE_DIGEST"
+
# The expression maps the explicit environment input to exactly one repo
</file context>
There was a problem hiding this comment.
4 issues found across 161 files
Confidence score: 1/5
- In
project-membership.sql, the deferred Project trigger rejects normal project creation because the Project is inserted before its first workspace. Adjust the check so it does not abort the transaction before the workspace exists. - In
backfill-repair.integration.ts, the barrier blocks the first eight workflows, leavingtailqueued and unnotified. Block only one slow request so another worker can reachtail. - In
push.integration.ts, the fixture cannot satisfy later reconcilers, and the assertion still passes when the push exits nonzero. Provide the required tables and assert a successful exit. - In
lifecycle.ts, a pub/sub subscriber throwing after provider cleanup makes strict archive repair rethrow a notification error as if provider cleanup failed. Keep notification failures separate from provider cleanup status.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/projects/__integration__/backfill-repair.integration.ts">
<violation number="1" location="apps/sim/lib/projects/__integration__/backfill-repair.integration.ts:38">
P2: This barrier prevents `tail` from being notified: the first eight sorted workflows (`flow` and `slow-1` through `slow-7`) all block, leaving `tail` queued. Block only one slow request so another worker can reach `tail` before the test releases the barrier.</violation>
</file>
<file name="packages/db/maintenance/project-membership.sql">
<violation number="1" location="packages/db/maintenance/project-membership.sql:242">
P0: This deferred Project trigger rejects the normal create flow: the application inserts the Project before its first workspace, so the Project event runs with zero environments and aborts the transaction. Defer the nonempty check until the workspace membership event/final state is available, while retaining a guard for genuinely empty Projects.</violation>
</file>
<file name="packages/db/scripts/push.integration.ts">
<violation number="1" location="packages/db/scripts/push.integration.ts:229">
P2: This fixture cannot complete the push wrapper because its later reconcilers require tables the schema does not create. The assertion only checks spawn errors, so it passes on a nonzero exit; provide the required fixtures or isolate Project reconciliation, then assert `status` is zero.</violation>
</file>
<file name="apps/sim/lib/workspaces/lifecycle.ts">
<violation number="1" location="apps/sim/lib/workspaces/lifecycle.ts:154">
P2: Strict archive repair currently treats MCP notification failures as unfinished provider cleanup. If a local pub/sub subscriber throws after provider cleanup succeeds, this branch rethrows the notification error, leaves the repair journal incomplete, and blocks migration completion; isolate best-effort MCP notification errors from the strict provider-cleanup result.</violation>
</file>
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
| CREATE TRIGGER project_contract_lock BEFORE INSERT OR UPDATE OR DELETE ON project | ||
| FOR EACH ROW EXECUTE FUNCTION project_contract_before_write(); | ||
| DROP TRIGGER IF EXISTS project_contract_check ON project; | ||
| CREATE CONSTRAINT TRIGGER project_contract_check AFTER INSERT OR UPDATE OR DELETE ON project |
There was a problem hiding this comment.
P0: This deferred Project trigger rejects the normal create flow: the application inserts the Project before its first workspace, so the Project event runs with zero environments and aborts the transaction. Defer the nonempty check until the workspace membership event/final state is available, while retaining a guard for genuinely empty Projects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/db/maintenance/project-membership.sql, line 242:
<comment>This deferred Project trigger rejects the normal create flow: the application inserts the Project before its first workspace, so the Project event runs with zero environments and aborts the transaction. Defer the nonempty check until the workspace membership event/final state is available, while retaining a guard for genuinely empty Projects.</comment>
<file context>
@@ -0,0 +1,268 @@
+CREATE TRIGGER project_contract_lock BEFORE INSERT OR UPDATE OR DELETE ON project
+FOR EACH ROW EXECUTE FUNCTION project_contract_before_write();
+DROP TRIGGER IF EXISTS project_contract_check ON project;
+CREATE CONSTRAINT TRIGGER project_contract_check AFTER INSERT OR UPDATE OR DELETE ON project
+DEFERRABLE INITIALLY DEFERRED FOR EACH ROW EXECUTE FUNCTION project_contract_after_write();
+--> statement-breakpoint
</file context>
| for await (const chunk of request) chunks.push(Buffer.from(chunk)) | ||
| const { workflowId } = JSON.parse(Buffer.concat(chunks).toString()) as { workflowId: string } | ||
| if (workflowId === 'tail') tailNotified = true | ||
| if (workflowId.startsWith('slow-')) await releaseNotifications.promise |
There was a problem hiding this comment.
P2: This barrier prevents tail from being notified: the first eight sorted workflows (flow and slow-1 through slow-7) all block, leaving tail queued. Block only one slow request so another worker can reach tail before the test releases the barrier.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apps/sim/lib/projects/__integration__/backfill-repair.integration.ts, line 38:
<comment>This barrier prevents `tail` from being notified: the first eight sorted workflows (`flow` and `slow-1` through `slow-7`) all block, leaving `tail` queued. Block only one slow request so another worker can reach `tail` before the test releases the barrier.</comment>
<file context>
@@ -0,0 +1,281 @@
+ for await (const chunk of request) chunks.push(Buffer.from(chunk))
+ const { workflowId } = JSON.parse(Buffer.concat(chunks).toString()) as { workflowId: string }
+ if (workflowId === 'tail') tailNotified = true
+ if (workflowId.startsWith('slow-')) await releaseNotifications.promise
+ response.writeHead(200, { 'content-type': 'application/json' }).end('{}')
+})
</file context>
| if (workflowId.startsWith('slow-')) await releaseNotifications.promise | |
| if (workflowId === 'slow-7') await releaseNotifications.promise |
| projectId: text('project_id').notNull().references(() => projects.id, { onDelete: 'restrict' }), | ||
| organizationId: text('organization_id'), archivedAt: timestamp('archived_at'), forkedFromWorkspaceId: text('forked_from_workspace_id'), | ||
| }) | ||
| export const workflows = pgTable('workflow', { |
There was a problem hiding this comment.
P2: This fixture cannot complete the push wrapper because its later reconcilers require tables the schema does not create. The assertion only checks spawn errors, so it passes on a nonzero exit; provide the required fixtures or isolate Project reconciliation, then assert status is zero.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/db/scripts/push.integration.ts, line 229:
<comment>This fixture cannot complete the push wrapper because its later reconcilers require tables the schema does not create. The assertion only checks spawn errors, so it passes on a nonzero exit; provide the required fixtures or isolate Project reconciliation, then assert `status` is zero.</comment>
<file context>
@@ -185,108 +187,71 @@ export const knowledgeBases = pgTable('knowledge_base', {
+ projectId: text('project_id').notNull().references(() => projects.id, { onDelete: 'restrict' }),
+ organizationId: text('organization_id'), archivedAt: timestamp('archived_at'), forkedFromWorkspaceId: text('forked_from_workspace_id'),
+})
+export const workflows = pgTable('workflow', {
+ id: text('id').primaryKey(), workspaceId: text('workspace_id'), archivedAt: timestamp('archived_at'),
})`)
</file context>
| requestId, | ||
| strictExternalCleanup: options.strictExternalCleanup, | ||
| }).catch((error: unknown) => { | ||
| if (options.strictExternalCleanup) { |
There was a problem hiding this comment.
P2: Strict archive repair currently treats MCP notification failures as unfinished provider cleanup. If a local pub/sub subscriber throws after provider cleanup succeeds, this branch rethrows the notification error, leaves the repair journal incomplete, and blocks migration completion; isolate best-effort MCP notification errors from the strict provider-cleanup result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apps/sim/lib/workspaces/lifecycle.ts, line 154:
<comment>Strict archive repair currently treats MCP notification failures as unfinished provider cleanup. If a local pub/sub subscriber throws after provider cleanup succeeds, this branch rethrows the notification error, leaves the repair journal incomplete, and blocks migration completion; isolate best-effort MCP notification errors from the strict provider-cleanup result.</comment>
<file context>
@@ -135,30 +135,44 @@ export async function archiveEnvironmentInTransaction(
+ requestId,
+ strictExternalCleanup: options.strictExternalCleanup,
+ }).catch((error: unknown) => {
+ if (options.strictExternalCleanup) {
+ cleanupErrors.push(error)
+ return
</file context>
This branch was successfully deployed
1 participant
Summary
workspace.project_idrollout after feat(projects): move Project membership to the workspace column #8830 has been deployed and older application and background-worker versions have drained. This PR is stacked on feat(projects): move Project membership to the workspace column #8830; the deployment preflight remains mandatory.0031_project_membershipin the existing TypeScript migration runner. It preserves legacy Project identities, gives populated columns precedence over stale connector rows, and backfills missing assignments in transactions of at most 50 singletons or one complete fork family. Bounded contention retries release locks, allow unrelated families to progress and resume from committed assignments.project_workspaceatomically under non-waiting table locks. It adds no Project trigger to ordinary workflow writes.Type of Change
Testing
Checklist
test-auditauthoring gate)