fix(chat): serialize rule updates - #3121
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
c863a5b to
497c23d
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found and verified against the latest diff
You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
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="apps/web/utils/rule/rule.ts">
<violation number="1" location="apps/web/utils/rule/rule.ts:236">
P2: Concurrent bulk/single-rule writes can produce incorrect duplicate history entries because the post-write query is not tied to rows affected by this write. Capturing the affected rows atomically or carrying an expected prior state through the write would keep history limited to this operation's changes.</violation>
</file>
<file name="apps/web/__tests__/eval/assistant-chat-rule-eval-test-utils.ts">
<violation number="1" location="apps/web/__tests__/eval/assistant-chat-rule-eval-test-utils.ts:22">
P3: mockSetRulesEnabled is added to the type and module mock builder but not to configureRuleMutationMocks, creating an inconsistency where the new mock lacks the default resolved value that all other rule mutation mocks receive through configureRuleMutationMocks.</violation>
</file>
<file name="apps/web/utils/ai/assistant/chat.ts">
<violation number="1" location="apps/web/utils/ai/assistant/chat.ts:270">
P3: The snapshot is loaded twice in the `writeResult.count !== changedRules.length` → `updateIsComplete === true` edge case. The first load verifies the update; the second loads the same data immediately after. Move the snapshot load outside the `if` block or early-return from the inner block to avoid the redundant query.</violation>
</file>
<file name="apps/web/utils/rule/rule.test.ts">
<violation number="1" location="apps/web/utils/rule/rule.test.ts:708">
P3: Add a test where `updateMany` returns `{ count: 0 }` (all rules already match the target state) and verify that `findMany` and `createRuleHistory` are not called.</violation>
<violation number="2" location="apps/web/utils/rule/rule.test.ts:709">
P3: Add an assertion verifying `prisma.rule.findMany` was called with the correct `where` (id `in` clause, `emailAccountId`, `enabled`) and `include: ruleHistoryRuleInclude`. Without it, a change that drops the `enabled` filter or the `include` in the `findMany` query would silently pass the test.</violation>
<violation number="3" location="apps/web/utils/rule/rule.test.ts:717">
P3: Add a test case for the enabling path (`enabled: true`) with non-empty rule IDs to verify the `updateMany` filter becomes `enabled: { not: true }` and `findMany` fetches rules with `enabled: true`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| export async function setRulesEnabled({ | ||
| ruleIds, | ||
| emailAccountId, | ||
| enabled, |
There was a problem hiding this comment.
P2: Concurrent bulk/single-rule writes can produce incorrect duplicate history entries because the post-write query is not tied to rows affected by this write. Capturing the affected rows atomically or carrying an expected prior state through the write would keep history limited to this operation's changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/utils/rule/rule.ts, line 236:
<comment>Concurrent bulk/single-rule writes can produce incorrect duplicate history entries because the post-write query is not tied to rows affected by this write. Capturing the affected rows atomically or carrying an expected prior state through the write would keep history limited to this operation's changes.</comment>
<file context>
@@ -230,6 +230,44 @@ export function setRuleEnabled({
+export async function setRulesEnabled({
+ ruleIds,
+ emailAccountId,
+ enabled,
+}: {
+ ruleIds: string[];
</file context>
There was a problem hiding this comment.
Good catch on the old implementation. The bulk setRulesEnabled helper and its history path have now been removed entirely. The PR instead serializes the existing per-rule updateRule executions, so there is no bulk/single post-write history query or duplicate-history scenario left in the current diff.
There was a problem hiding this comment.
Commit d11e5ea addressed this comment by removing the bulk update/history path and serializing existing per-rule updates. This prevents concurrent writes from producing duplicate or unrelated history entries.
| mockPartialUpdateRule: AnyMock; | ||
| mockUpdateRuleActions: AnyMock; | ||
| mockSetRuleEnabled?: AnyMock; | ||
| mockSetRulesEnabled?: AnyMock; |
There was a problem hiding this comment.
P3: mockSetRulesEnabled is added to the type and module mock builder but not to configureRuleMutationMocks, creating an inconsistency where the new mock lacks the default resolved value that all other rule mutation mocks receive through configureRuleMutationMocks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/__tests__/eval/assistant-chat-rule-eval-test-utils.ts, line 22:
<comment>mockSetRulesEnabled is added to the type and module mock builder but not to configureRuleMutationMocks, creating an inconsistency where the new mock lacks the default resolved value that all other rule mutation mocks receive through configureRuleMutationMocks.</comment>
<file context>
@@ -19,6 +19,7 @@ type RuleMutationMocks = {
mockPartialUpdateRule: AnyMock;
mockUpdateRuleActions: AnyMock;
mockSetRuleEnabled?: AnyMock;
+ mockSetRulesEnabled?: AnyMock;
};
</file context>
There was a problem hiding this comment.
The setRulesEnabled tool and all associated eval mock plumbing, including mockSetRulesEnabled, have now been removed. The current eval exercises two calls to the existing updateRule tool, so this inconsistency no longer exists in the PR.
There was a problem hiding this comment.
Commit d11e5ea addressed this comment by removing the setRulesEnabled mock plumbing entirely. The eval now uses existing updateRule calls instead, so the inconsistent default mock configuration no longer exists.
| getLearnedPatterns: getLearnedPatternsTool(toolOptions), | ||
| createRule: createRuleTool(toolOptions), | ||
| updateRule: updateRuleTool(toolOptions), | ||
| setRulesEnabled: setRulesEnabledTool(toolOptions), |
There was a problem hiding this comment.
P3: The snapshot is loaded twice in the writeResult.count !== changedRules.length → updateIsComplete === true edge case. The first load verifies the update; the second loads the same data immediately after. Move the snapshot load outside the if block or early-return from the inner block to avoid the redundant query.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/utils/ai/assistant/chat.ts, line 270:
<comment>The snapshot is loaded twice in the `writeResult.count !== changedRules.length` → `updateIsComplete === true` edge case. The first load verifies the update; the second loads the same data immediately after. Move the snapshot load outside the `if` block or early-return from the inner block to avoid the redundant query.</comment>
<file context>
@@ -266,6 +267,7 @@ export async function aiProcessAssistantChat({
getLearnedPatterns: getLearnedPatternsTool(toolOptions),
createRule: createRuleTool(toolOptions),
updateRule: updateRuleTool(toolOptions),
+ setRulesEnabled: setRulesEnabledTool(toolOptions),
deleteRule: deleteRuleTool(toolOptions),
updateLearnedPatterns: updateLearnedPatternsTool(toolOptions),
</file context>
There was a problem hiding this comment.
The bulk setRulesEnabled tool and its snapshot-verification path have now been removed. The only current change in chat.ts is the pipeline version bump; serialization lives inside the existing updateRule tool, which refreshes its snapshot once after each update.
There was a problem hiding this comment.
Commit d11e5ea addressed this comment by removing the bulk setRulesEnabled tool and its snapshot-verification path entirely. Rule updates now serialize through updateRule, which refreshes the snapshot once per update.
| beforeEach(resetRuleMocks); | ||
|
|
||
| it("updates all enabled states in one write and records each history", async () => { | ||
| prisma.rule.updateMany.mockResolvedValue({ count: 2 }); |
There was a problem hiding this comment.
P3: Add a test where updateMany returns { count: 0 } (all rules already match the target state) and verify that findMany and createRuleHistory are not called.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/utils/rule/rule.test.ts, line 708:
<comment>Add a test where `updateMany` returns `{ count: 0 }` (all rules already match the target state) and verify that `findMany` and `createRuleHistory` are not called.</comment>
<file context>
@@ -700,6 +701,56 @@ describe("rule history snapshots", () => {
+ beforeEach(resetRuleMocks);
+
+ it("updates all enabled states in one write and records each history", async () => {
+ prisma.rule.updateMany.mockResolvedValue({ count: 2 });
+ prisma.rule.findMany.mockResolvedValue([
+ { id: "rule-one", actions: [], group: null },
</file context>
There was a problem hiding this comment.
The setRulesEnabled domain helper and its tests have now been removed from the PR, so the updateMany count-zero branch no longer exists. The replacement regression test covers concurrent calls through the existing updateRule tool.
There was a problem hiding this comment.
Commit d11e5ea addressed this comment by removing the setRulesEnabled helper and its count-zero branch entirely. A replacement regression test now covers concurrent updates through the existing updateRule tool.
| const result = await setRulesEnabled({ | ||
| ruleIds: ["rule-one", "rule-two"], | ||
| emailAccountId: EMAIL_ACCOUNT_ID, | ||
| enabled: false, |
There was a problem hiding this comment.
P3: Add a test case for the enabling path (enabled: true) with non-empty rule IDs to verify the updateMany filter becomes enabled: { not: true } and findMany fetches rules with enabled: true.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/utils/rule/rule.test.ts, line 717:
<comment>Add a test case for the enabling path (`enabled: true`) with non-empty rule IDs to verify the `updateMany` filter becomes `enabled: { not: true }` and `findMany` fetches rules with `enabled: true`.</comment>
<file context>
@@ -700,6 +701,56 @@ describe("rule history snapshots", () => {
+ const result = await setRulesEnabled({
+ ruleIds: ["rule-one", "rule-two"],
+ emailAccountId: EMAIL_ACCOUNT_ID,
+ enabled: false,
+ });
+
</file context>
There was a problem hiding this comment.
The setRulesEnabled helper and updateMany path have now been removed entirely. Enable and disable requests both continue through the existing updateRule tool, so this bulk-path test case is no longer applicable.
There was a problem hiding this comment.
Commit d11e5ea addressed this comment by removing the setRulesEnabled helper and bulk updateMany path entirely. Enable/disable operations now use updateRule, with coverage for multiple rule updates.
|
|
||
| it("updates all enabled states in one write and records each history", async () => { | ||
| prisma.rule.updateMany.mockResolvedValue({ count: 2 }); | ||
| prisma.rule.findMany.mockResolvedValue([ |
There was a problem hiding this comment.
P3: Add an assertion verifying prisma.rule.findMany was called with the correct where (id in clause, emailAccountId, enabled) and include: ruleHistoryRuleInclude. Without it, a change that drops the enabled filter or the include in the findMany query would silently pass the test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/utils/rule/rule.test.ts, line 709:
<comment>Add an assertion verifying `prisma.rule.findMany` was called with the correct `where` (id `in` clause, `emailAccountId`, `enabled`) and `include: ruleHistoryRuleInclude`. Without it, a change that drops the `enabled` filter or the `include` in the `findMany` query would silently pass the test.</comment>
<file context>
@@ -700,6 +701,56 @@ describe("rule history snapshots", () => {
+
+ it("updates all enabled states in one write and records each history", async () => {
+ prisma.rule.updateMany.mockResolvedValue({ count: 2 });
+ prisma.rule.findMany.mockResolvedValue([
+ { id: "rule-one", actions: [], group: null },
+ { id: "rule-two", actions: [], group: null },
</file context>
There was a problem hiding this comment.
The post-update findMany query belonged to the removed setRulesEnabled helper. That helper and its tests are no longer in the current diff; the PR now serializes ordinary updateRule calls instead, so this assertion is no longer applicable.
There was a problem hiding this comment.
Commit d11e5ea addressed this comment by removing the bulk setRulesEnabled helper and its post-update findMany query. Updates now use serialized ordinary updateRule calls, so the requested assertion is no longer applicable.
497c23d to
524eabe
Compare
elie222
changed the title
524eabe to
d11e5ea
Compare
d11e5ea to
c3d8a3d
Compare
1 participant
Stack created with GitHub Stacks CLI • 4 of 4
Summary
updateRuletool as the single way to edit an existing rule.updateRuleexecutions within one assistant run so each update validates against the rule revision produced by the previous update.Why
The assistant could already emit one
updateRulecall per requested rule. Multiple calls from the same model step could execute concurrently, however, so the first write advanced the account rules revision while a sibling call was still validating stale state. Runtime serialization fixes that race without adding a special-case bulk tool.Testing
Risks
updateRulecall to finish; a slow first update delays later ones.updateRulecalls in one run and does not coordinate different rule tool types or separate requests.