feat(forks): opt-in fork sync for new workflows - #8318
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
All reported issues were addressed across 42 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
d3701ae to
45181c2
Compare
|
@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 42 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
There was a problem hiding this comment.
No issues found across 42 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
Workspace forks gain a lineage-wide policy for whether a NEWLY created workflow joins fork sync. Today a workflow joins the moment it is deployed, so deploying something experimental in a parent pushes it into every fork on the next sync with no step where anyone chose that. Opt-out remains the default, so no existing or new workspace changes behaviour until someone flips the toggle. The policy lives on workspace.fork_sync_new_workflows_excluded (default false, the historical behaviour); workflow.fork_sync_excluded is untouched including its default. It is uniform across a fork lineage: settable from any member, fanning out to every ancestor and descendant under an advisory lock keyed on the lineage root, which fork creation and unlink also take. It is forward-only - flipping it never rewrites an existing workflow's sync state - and each changed member records its own audit entry naming where the change was issued from. Genuinely new workflows (create, duplicate, admin/superuser import, a fork's starter) take the workspace policy. A copy (fork creation, promote-create) inherits the SOURCE workflow's flag, because it is the same logical workflow in another workspace; without that an opted-in workflow would copy into a fork already excluded and never sync again. The fork modal gains "Copy unsynced workflows" (off by default, shown only when the source has unsynced deployed workflows, disabled when the combined set would exceed the fork ceiling) so forking an opt-in workspace cannot silently produce an empty fork. The settings section becomes "Synced workflows" with the polarity flipped - checked means the workflow syncs - above a "Sync new workflows by default" toggle row that states its lineage-wide reach. The wire field stays forkSyncExcluded, so the tree owns the single inversion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
45181c2 to
9b4dcde
Compare
|
@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 44 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Fix all with cubic | Re-trigger cubic
…once Round 3 review left 7 threads. Five were the same mistake: a guard added at one read site of a value while the other read sites kept the unguarded one. Each fix below is one derived value used everywhere, or a deleted duplicate path, rather than another guard at another call site. Lock-order deadlock (reported independently by both reviewers). `lineage.ts` already declared `fork-lineage` the coarsest fork lock, but ranked only the three advisory locks and said nothing about `lockForkRevision`, which takes FOR UPDATE on `workspace`. `createFork` took it first, so it held that row while waiting for `fork-lineage`, while `unlinkForkEdge` held `fork-lineage` and waited to UPDATE the same row. Hoist the lineage lock above it, and replace the partial contract with a rank table covering every lock in the module. All six fork transactions now acquire in ascending rank. Stale workspace rows in the synced-workflows list. `useWorkflows` and `useFolders` both set `placeholderData: keepPreviousData`, so a workspace switch served the previous workspace's rows with `isLoading: false` and a click posted workspace A's ids against B. Gate on `isPending || isPlaceholderData`, matching `custom-tools.tsx`. Fork modal submitted a value the switch showed as off. `copyUnsyncedWorkflows` had two readings and submit used the raw one. Derive it once from the request plus the limit, and read that at all four sites. Audit entries named workspaces by id. Return the name from the UPDATE and project `resourceName` from it, so a lineage-wide change no longer reads as one named workspace and N opaque identifiers. Also: give `setForkSyncDefault`'s multi-row UPDATE a deterministic row-lock order, delete the non-barrel re-export left behind when the workflow limit moved to `limits.ts`, and correct a TSDoc invariant that claimed the `fork-target` lock for both callers when only one holds it. Tests, per the repo's test-audit gate: add one `*.integration.ts` that races the two real lock sequences against real Postgres and asserts the pre-fix order deadlocks while the shipped order does not, so the check cannot pass vacuously. Drop the re-added contract-schema test (an identical file was pruned as low-signal in #8295), fold the fork-sync inheritance assertion into its sibling, and reduce the synced-workflows test to the pure tree builder whose checkbox stub no longer implements the polarity it asserts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@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 47 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Fix all with cubic | Re-trigger cubic
cubic's round-4 finding on the new integration suite was right: it mirrored the lock SQL as string constants instead of executing production's, so a `createFork` regression to the pre-fix order would have left it green. Closed on both halves. The fixture now calls the real `setForkLockTimeout`, `acquireForkLineageLock` and `lockForkRevision`, against the tables the last of those actually locks, so a change to the advisory-lock key or to the revision lock's coverage is carried into the test rather than silently diverging from a copy. A third check pins why the pair conflicts at all: the revision lock really does hold the `workspace` row, which is the edge of the cycle. The order inside `createFork` is asserted where it lives, in `create-fork.test.ts`, on invocation order through the admission path. Order is the entire contract here, which is the case the retention bar keeps call ordering for. Verified red for the right reason: reordering the two acquisitions in `create-fork.ts` fails the new unit assertion, and the integration suite's negative control still fails in ~1.03s with the server reporting `deadlock detected` rather than a lock timeout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Unchecking a workflow meant "keep it out of forking entirely", and the docs said so: never sent, never received, never copied into a new fork. The override made the last of those three conditional on a modal toggle. Sync participation was never at risk - copies inherited the source's flag, so an overridden copy landed unsynced and could not sync back - but "never copied into a new fork" stopped being unconditional, and that is the guarantee someone is relying on when they uncheck a workflow. It also conflated two different states. "Unsynced" covers both "I deliberately excluded this" and "this was never checked because the lineage default is off", and the override copied both. The first case is the one the guarantee exists for. This restores the single predicate: `forkSyncExcluded` workflows are invisible to fork creation, the diff preview, promote in both directions, and the mapping scan, with no caller able to lift it. Removed the toggle and its section, `copyUnsyncedWorkflows` from the request contract, `includeSyncExcluded` from `listDeployedWorkflows` and `loadSourceDeployedStates`, the `unsyncedDeployedWorkflowCount` field, and the split count query, which goes back to one count carrying the exclusion predicate. `lib/limits.ts` goes too. It existed only so a client component could read `MAX_FORK_DEPLOYED_WORKFLOWS` without pulling the database client into the browser bundle, and the override's toggle was the only client reader. The constant returns to `copy/deploy-bridge.ts`, where it is enforced. Unaffected, and still the point of the feature: the lineage-wide new-workflow default, its forward-only write, and the rule that a genuinely new workflow takes the workspace policy while a copy inherits its source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@greptile review |
|
@cubic review |
@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 42 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.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Fix all with cubic | Re-trigger cubic
`listDeployedWorkflows` filters on `fork_sync_excluded = false` and then selected the same column, so every row it returned carried `false` by construction - `SELECT x ... WHERE x = false`. The field only ever varied while `includeSyncExcluded` could make that predicate drop out, which the previous commit removed, so it is scaffolding from the reverted override rather than anything load-bearing. Keeping it would not have been defensive either. If someone widens that predicate they have to revisit the projection anyway, and a constant field hides the coupling between the two instead of enforcing it. Dropped from the query and from `DeployedWorkflowSummary`. The two write sites now state the invariant they actually mean: a copy is not a new workflow, so it is written synced and never takes the target workspace's new-workflow default - which in an opt-out lineage would land a deliberately synced workflow unsynced on the other side. That explicit write is kept precisely because it is a semantic claim, not a read of something the query had already decided. Untouched: the target-side read in `promote-plan.ts`, which queries target workflows with no exclusion filter and genuinely varies. That is what keeps a promote from overwriting a target the user unchecked. Dropped the copy test for an unsynced source, an input no caller can now produce, and renamed its sibling to the property that still holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@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 41 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.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
`finishes a pass within its page budget` times out against the shared 30s default on a loaded CI runner. It is a load flake, not a regression: the same commit went green on the `push` integration job and timed out on `migrate`, and it runs in ~4s locally. The cost is real work, not a hang. The test seeds `MEMBER_TOMBSTONE_RECONCILE_PAGES_PER_RUN * 500 + 500` documents specifically so one pass cannot finish inside a single run's budget, then observes and re-lists all of it. That volume is the assertion, so trimming it to fit the default would stop proving the multi-run path. Given its own 120s budget instead, matching the per-test timeouts already used elsewhere in this directory, with a comment recording why. Unrelated to this branch's fork-sync work - the file is byte-identical to staging and arrived with the merge - but it was failing this PR's CI, and a flake left alone becomes one everybody learns to ignore. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@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 42 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.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
CLAUDE.md makes the chip family the canonical control chrome, and `ChipSwitch` is what the equivalent settings row already uses (`inbox-enable-toggle.tsx`). Two knock-on details, both forced by the control rather than chosen. `ChipSwitch` is a Radix radio group over a string, so the boolean inversion now runs through named values instead of `!`: `exclude` and `sync` map onto the stored `forkSyncNewWorkflowsExcluded`. And it takes no `id`, so the `Label` drops its `htmlFor` and the group carries its own `aria-label` - the same pairing `inbox-enable-toggle.tsx` uses. It also reads better here. This row's "off" means new workflows stop syncing across the whole lineage, which a thumb position leaves the reader to infer from the label; naming both outcomes puts it on screen. No behavior change beyond the control: the same mutation, the same error toast, the same placeholder-data gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@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 42 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.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
|
||
| beforeEach(() => { | ||
| resetDbChainMock() | ||
| vi.clearAllMocks() |
There was a problem hiding this comment.
This beforeEach calls vi.clearAllMocks(), but the shared Vitest configuration already clears mock history before every test. The repository testing instructions explicitly prohibit calling it in hooks, so this requirement must be satisfied before merging.
| vi.clearAllMocks() |
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
Workspace forks gain an opt-in policy for new workflows. Today a workflow joins fork sync the moment it is deployed, so deploying something experimental in a parent workspace pushes it into every fork on the next sync with no step where anyone chose that.
Opt-out remains the default — no existing or new workspace changes behaviour until someone flips the toggle.
The policy
workspace.fork_sync_new_workflows_excludedcolumn (defaultfalse, the historical behaviour).workflow.fork_sync_excludedis untouched, including its default.Where it applies
Fork creation
Settings UI
forkSyncExcluded, so the tree owns the single inversion.Type of Change
Testing
Automated — 244 files / 3014 tests passing (2972 → 3014, +42 new), 51/51 audits,
type-checkclean across workspaces,@sim/appproduction build green.setForkSyncDefault(fan-out, lock, audit, authz)Two assertions were mutation-tested (removing the fix they guard makes them fail), because both guard bugs that had already slipped through a passing test once.
Manual, end to end against a live stack, on the merged tree, with a scratch database migrated from empty so
0385was proven to apply on top of0384:willCreate: 0); checking it moves it towillCreate: 1and a promote lands it.excluded = falsein a target whose policy istrue— the inherit rule doing its job.What reviewers should focus on
copy-workflows.ts— a copy inheriting the source's flag rather than the target workspace's policy. This is the load-bearing rule; get it wrong and opted-in workflows silently stop syncing.sync-default.ts— lineage traversal must resolve the same root and member set from every member, including across archived workspaces, or two callers take different advisory locks for one lineage.Known, not fixed — raised in review, tracked for follow-up:
assertForkSourceVersionshard-codesfork_sync_excluded = false, so the copy-unsynced override sits outside the preview-freshness guarantee. Unreachable today (the v2 fork contract is.strict()and never gained the field, and the internal route sends no admission) but a trap for whoever adds it.copyUnsyncedWorkflows, sopreviewWorkspaceFork's branch for it is currently dead.workspace.fork_sync_default_changedhas no docs row —audit-logs.mdxlists no Forks events at all today, so that is separate scope.Checklist
Screenshots/Videos
Forks settings — "Synced workflows" with the new default toggle (checked = syncs;
IT-Agentdeliberately unchecked):Fork modal — the override, shown only when unsynced deployed workflows exist:
Verified in light and dark at 1440px.
Documentation
apps/docs/content/docs/platform/enterprise/forks.mdx— the public Forks page, synced to the opt-in policy: theExcluded workflows→Synced workflowsrename with the polarity flip throughout, a newSync new workflows by defaultsubsection covering the three easy-to-miss properties (lineage-wide write, forward-only, and what counts as "new" versus a copy), the fork modal'sCopy unsynced workflowstoggle, updated resource-reference, permissions, edge-case and FAQ rows, and both#excluded-workflowsanchors repointed.Deployment Notes
Migration
0385is oneADD COLUMNwith a constant default onworkspace, non-rewriting in modern Postgres. No backfill: every existing workspace lands on the value matching its current behaviour, so every existing lineage is uniform on arrival.🤖 Generated with Claude Code