chore(tests): remove low-signal tests and consolidate the testing setup - #8295
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3000 files
Confidence score: 5/5
- The Vitest command documented in
.claude/rules/sim-testing.mdmay point to a missing binary and make the simulation test instructions fail; verify the relative path fromapps/simbefore relying on it.
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=".claude/rules/sim-testing.md">
<violation number="1" location=".claude/rules/sim-testing.md:125">
P3: `../../node_modules/.bin/vitest` run from `apps/sim` resolves one directory above the repository root (`apps/sim` is directly under the root), so it points at a path that does not exist. The hoisted binary is at the repo root: `../node_modules/.bin/vitest run <paths>`.</violation>
</file>
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
…is suite, restored client-info wire contract
Collaborator
Author
|
Restored the |
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review this PR |
Contributor
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
1 issue found across 3000 files
Confidence score: 4/5
.agents/skills/test-audit/SKILL.mdmay reject valid behavior-focused unit and regression tests solely because they were added after implementation, including tests that expose the intended regression; allow tests before or after code and assess them against the contract.
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=".agents/skills/test-audit/SKILL.md">
<violation number="1" location=".agents/skills/test-audit/SKILL.md:20">
P2: This blanket rule blocks valid behavior-focused unit and regression tests added after implementation, even when they fail for the intended regression. Allow tests before or after code and judge them by the contract they protect and whether they fail when the fix is reverted.</violation>
</file>
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Fix all with cubic | Re-trigger cubic
This was referenced Sep 25, 2026
mzxchandra
added a commit
that referenced
this pull request
Sep 28, 2026
…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>
mzxchandra
added a commit
that referenced
this pull request
Sep 29, 2026
* feat(forks): opt-in fork sync for new workflows 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> * fix(forks): break the fork lock cycle and derive review-round values 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> * test(forks): drive the lock-order suite through the production helpers 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> * test(forks): scope the lock-order barrier to the unlink backend cubic was right that counting any Lock waiter on the database could let an unrelated session release the barrier before the unlink had blocked, leaving the pre-fix negative control to run with its cycle still open. A weak negative control is the failure mode this suite exists to avoid. The unlink transaction now publishes its own backend pid before it can block, and the barrier waits on exactly that pid. If it never blocks the race did not set up, so it throws rather than quietly proceeding and reporting a pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(forks): settle every lock-order barrier on the failure path Both reviewers independently caught the same real defect: a session that died before resolving its barrier left `raceForkAgainstUnlink` awaiting a deferred that would never settle, so a clean database error surfaced as a 30-second vitest timeout. That hides exactly the diagnostics a concurrency test exists to provide. Every barrier is now settled on the failure path as well as the happy one. The fork session releases `forkHoldsFirstLock` in a `finally`, the unlink session reports a `null` backend when it dies before `pg_backend_pid()` returns, and the barrier is skipped outright once a session has already failed, so the session's own error is what the test reports. Releasing the sessions and draining them moved into a `finally` too, along with closing both connections, so a throw inside the barrier cannot strand the fork transaction on `unlinkMayFinish`. Verified by simulating the failure they described - a fork session that throws before taking its first lock. It now fails in 1s with "simulated early connection failure" instead of timing out at 30s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(forks): drop call-count assertions from the sync-default suite CLAUDE.md forbids tests that assert a mock was called. Three assertions in this suite were exactly that shape - `mock.calls).toHaveLength(0)` on the audit and analytics mocks - and they proved only what the mock itself decided, not anything about the use case. The no-op case now asserts the result instead: `projectAudit` maps over `changedWorkspaces` and `afterSuccess` returns early when it is empty, so an empty result IS "no audit, no analytics", pinned to the value the fan-out actually reads. The admission case keeps its rejection assertion, which is the real guarantee: admission runs before the transaction opens. What stays reads the CONTENT of the entries filed - each one carried by its own workspace id and name - which is a fan-out invariant type-check cannot see and the reason this suite exists. Confirmed it still goes red on the pre-fix code: reverting the audit-name guard fails with "expected 'root-ws' to be 'Name of root-ws'". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * revert(forks): drop the "Copy unsynced workflows" fork override 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> * refactor(forks): stop projecting a column the source query already pins `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> * test(knowledge): give the member-tombstone budget test its own timeout `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> * refactor(forks): render the fork-sync default with ChipSwitch 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> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
validateStringParam,validateRequiredFields,resubscribe,resolveSelectExportValue,getSlackV2ToolAccess, unused provider/model helpers,USAGE_PROVIDER_ICON_IDS)@sim/testingexports (unused assertions, builders, factories, mock helpers,setup/)*.postgres.test.ts→*.integration.ts, oneTEST_DATABASE_URL/TEST_REDIS_URLcontract enforced bypackages/db/testing/test-infrastructure.ts, CI discovers suites by glob instead of a hand-maintained file list, and every integration run uploads a JSON report.*.live.test.tsholds opt-in provider/sandbox suites (never in CI); 7 integration suites that already failed on a fresh DB are quarantined explicitly inapps/sim/vitest.config.tsminimumReleaseAgewindow); publish workflows run tests on Node 22 and keep a Node 20 bundle smoke@vitest-environment nodedocblocks (node is the default) and ~2,260vi.clearAllMocks()inbeforeEach(Vitest 5 clears mocks before every test)test-auditskill (authoring gate, junk patterns, retention bar, audit/campaign workflow), rewrite.claude/rules/sim-testing.mdaround test layers and naming, and gate new tests in/shipand/cleanupType of Change
Testing
bun run test(every workspace + scripts + setup): all green except 2 pre-existingbackground/knowledge-processing.test.tscases that fail only when the checkout path contains/privatepushandmigrateprovisioning: every file that ran in CI before still passes; SCIM HTTP e2e 17/17bunx turbo run type-check,bun run lint,bun run check:audits(49),docs-manifest:check, block registry,check:migrationsall passGreptile will likely refuse this PR for file count; the diff is almost entirely test deletions.
Checklist