Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
github-actions
Bot
added
the
requires-mothership-merge
|
![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 245 files
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.
Re-trigger cubic
ff42b48 to
7931397
Compare
|
@cubic-dev-ai review this PR |
@Sg312 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
5 issues found across 252 files
Confidence score: 2/5
- Plan eligibility revocation is not enforced on two paths:
organization-chats.tscan still serve persisted Plan chats, andrun-stage.tscan publish a generated spec after access is revoked. Recheck eligibility in both paths. - In
computer-tool-execution.ts, a transient failure of the page-exit completion request leaves the server-side waiter stranded after unload removes its execution entry. Add a retry or recovery path for the completion request. admin-move.tscan miss a chat created after its cleanup update, leaving that chat with the sourcememorySpaceId. Serialize chat creation with the move or add a follow-up cleanup.- In
computer-use.tsx, an older permission refresh can overwrite a newer revoke result and make a revoked app reappear. Ignore stale responses or serialize refreshes.
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/workspaces/admin-move.ts">
<violation number="1" location="apps/sim/lib/workspaces/admin-move.ts:1366">
P2: This one-shot cleanup is racy with chat creation: a chat that resolves the source organization before the move can be inserted after this `UPDATE` commits and retain the source `memorySpaceId`. Serialize chat creation with the workspace move or re-read the workspace organization/selection at insert time so post-move chats cannot retain an invalid graph binding.</violation>
</file>
<file name="apps/sim/lib/mothership/chat/organization-chats.ts">
<violation number="1" location="apps/sim/lib/mothership/chat/organization-chats.ts:76">
P2: Persisted Plan chats bypass this eligibility check when loaded through the chat-detail and organization-page paths, because those callers omit `input.mode`; a user whose Plan eligibility is revoked can still read the Plan conversation. Authorize against the persisted chat mode rather than only the caller-supplied mode.</violation>
</file>
<file name="apps/sim/app/workspace/[workspaceId]/settings/components/desktop/computer-use.tsx">
<violation number="1" location="apps/sim/app/workspace/[workspaceId]/settings/components/desktop/computer-use.tsx:31">
P3: An in-flight permission refresh can overwrite a newer revoke result because `.then(setApps)` accepts every response. Ignore stale list responses or serialize the refreshes so a revoked app cannot reappear in the approved-apps list.</violation>
</file>
<file name="apps/sim/lib/benchmarks/application/run-stage.ts">
<violation number="1" location="apps/sim/lib/benchmarks/application/run-stage.ts:237">
P2: The final reauthorization does not recheck the selected target’s Plan eligibility. If Super User/Plan access is revoked while the planner runs, this path still publishes its generated spec; reauthorize the target’s Plan access before completion.</violation>
</file>
<file name="apps/sim/lib/mothership/tools/client/computer-tool-execution.ts">
<violation number="1" location="apps/sim/lib/mothership/tools/client/computer-tool-execution.ts:139">
P1: A transient failure of the single page-exit completion request strands the server-side computer-tool waiter because unloading destroys the in-memory `executions` entry and the catch only logs the error. Mirror the browser tool’s unload path by sending a compact result through `sendBeacon`, falling back to keepalive, and retaining the completion for redelivery when both fail.</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.
Re-trigger cubic
| } | ||
| const onPageHide = () => { | ||
| cancel() | ||
| void reportClientToolCompletionOnPageExit( |
There was a problem hiding this comment.
P1: A transient failure of the single page-exit completion request strands the server-side computer-tool waiter because unloading destroys the in-memory executions entry and the catch only logs the error. Mirror the browser tool’s unload path by sending a compact result through sendBeacon, falling back to keepalive, and retaining the completion for redelivery when both fail.
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/mothership/tools/client/computer-tool-execution.ts, line 139:
<comment>A transient failure of the single page-exit completion request strands the server-side computer-tool waiter because unloading destroys the in-memory `executions` entry and the catch only logs the error. Mirror the browser tool’s unload path by sending a compact result through `sendBeacon`, falling back to keepalive, and retaining the completion for redelivery when both fail.</comment>
<file context>
@@ -0,0 +1,211 @@
+ }
+ const onPageHide = () => {
+ cancel()
+ void reportClientToolCompletionOnPageExit(
+ toolCallId,
+ ASYNC_TOOL_CONFIRMATION_STATUS.error,
</file context>
|
|
||
| // Private graph contents stay with their original organization when a workspace moves. | ||
| await tx | ||
| .update(copilotChats) |
There was a problem hiding this comment.
P2: This one-shot cleanup is racy with chat creation: a chat that resolves the source organization before the move can be inserted after this UPDATE commits and retain the source memorySpaceId. Serialize chat creation with the workspace move or re-read the workspace organization/selection at insert time so post-move chats cannot retain an invalid graph binding.
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/admin-move.ts, line 1366:
<comment>This one-shot cleanup is racy with chat creation: a chat that resolves the source organization before the move can be inserted after this `UPDATE` commits and retain the source `memorySpaceId`. Serialize chat creation with the workspace move or re-read the workspace organization/selection at insert time so post-move chats cannot retain an invalid graph binding.</comment>
<file context>
@@ -1360,6 +1361,17 @@ export async function moveWorkspaceToOrganization(params: {
+ // Private graph contents stay with their original organization when a workspace moves.
+ await tx
+ .update(copilotChats)
+ .set({ memorySpaceId: null })
+ .where(
</file context>
| if (input.mode === 'agent' || input.mode === 'plan') await requireBuildPermission(context) | ||
| if (input.mode === 'agent' || input.mode === 'plan') | ||
| await requireOrganizationBuildPermission(context) | ||
| if (input.mode === 'plan' && !(await isPlanModeEnabled(context.userId))) |
There was a problem hiding this comment.
P2: Persisted Plan chats bypass this eligibility check when loaded through the chat-detail and organization-page paths, because those callers omit input.mode; a user whose Plan eligibility is revoked can still read the Plan conversation. Authorize against the persisted chat mode rather than only the caller-supplied mode.
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/mothership/chat/organization-chats.ts, line 76:
<comment>Persisted Plan chats bypass this eligibility check when loaded through the chat-detail and organization-page paths, because those callers omit `input.mode`; a user whose Plan eligibility is revoked can still read the Plan conversation. Authorize against the persisted chat mode rather than only the caller-supplied mode.</comment>
<file context>
@@ -66,7 +71,10 @@ export const authorizeOrganizationChat = {
- if (input.mode === 'agent' || input.mode === 'plan') await requireBuildPermission(context)
+ if (input.mode === 'agent' || input.mode === 'plan')
+ await requireOrganizationBuildPermission(context)
+ if (input.mode === 'plan' && !(await isPlanModeEnabled(context.userId)))
+ throw new OrchestrationError('not_found', 'Plan mode is unavailable')
return context
</file context>
| (signal) => performStage(principal, claimed, input.stage, signal) | ||
| ) | ||
| request?.signal?.throwIfAborted() | ||
| await requireBenchmarkCaseAccess(principal, input) |
There was a problem hiding this comment.
P2: The final reauthorization does not recheck the selected target’s Plan eligibility. If Super User/Plan access is revoked while the planner runs, this path still publishes its generated spec; reauthorize the target’s Plan access before completion.
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/benchmarks/application/run-stage.ts, line 237:
<comment>The final reauthorization does not recheck the selected target’s Plan eligibility. If Super User/Plan access is revoked while the planner runs, this path still publishes its generated spec; reauthorize the target’s Plan access before completion.</comment>
<file context>
@@ -0,0 +1,271 @@
+ (signal) => performStage(principal, claimed, input.stage, signal)
+ )
+ request?.signal?.throwIfAborted()
+ await requireBenchmarkCaseAccess(principal, input)
+ return {
+ benchmark: await completeBenchmarkStage({
</file context>
| if (!bridge || !availability.data?.enabled) return | ||
| void bridge | ||
| .listAppPermissions() | ||
| .then(setApps) |
There was a problem hiding this comment.
P3: An in-flight permission refresh can overwrite a newer revoke result because .then(setApps) accepts every response. Ignore stale list responses or serialize the refreshes so a revoked app cannot reappear in the approved-apps list.
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/app/workspace/[workspaceId]/settings/components/desktop/computer-use.tsx, line 31:
<comment>An in-flight permission refresh can overwrite a newer revoke result because `.then(setApps)` accepts every response. Ignore stale list responses or serialize the refreshes so a revoked app cannot reappear in the approved-apps list.</comment>
<file context>
@@ -0,0 +1,122 @@
+ if (!bridge || !availability.data?.enabled) return
+ void bridge
+ .listAppPermissions()
+ .then(setApps)
+ .catch(() => toast.error('Could not load approved apps'))
+ }, [bridge, availability.data?.enabled, status?.activeAction])
</file context>
7931397 to
c0bf037
Compare
0f35b12 to
c88d1a8
Compare
|
@cubic-dev-ai review this PR |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 261 files
Confidence score: 3/5
- In
computer-tool-execution.ts, an 8 MiB screenshot can exceed the completion endpoint’s JSON body cap, and retries send the same oversized image, so the task may never complete. Bound or omit oversized screenshots before submitting them. - In
benchmark.tsx, a failed “Load older benchmarks” request hides already-loaded results and the retry button. Keep cached pages visible and allow the request to be retried. - In
native-client.ts, oversized requests rejected byComputerUse.swiftuse an error ID this client ignores, so calls can time out for schema-valid text with enough escaped control characters. Handleinvalid-requestresponses. - In
benchmark-score-chart.tsx, screen-reader users cannot tell which run is selected because the buttons expose no selected state. Addaria-pressedto the selected button.
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/app/o/[organizationId]/benchmark/benchmark.tsx">
<violation number="1" location="apps/sim/app/o/[organizationId]/benchmark/benchmark.tsx:68">
P2: A failed “Load older benchmarks” request sets `error` but retains earlier pages, so this branch hides the list and retry button. Keep cached pages visible on next-page errors and let the button retry.</violation>
</file>
<file name="apps/desktop/src/main/computer-use/native-client.ts">
<violation number="1" location="apps/desktop/src/main/computer-use/native-client.ts:48">
P2: `ComputerUse.swift` rejects requests over 128 KiB under ID `invalid-request`, which this client ignores. Schema-valid text with enough escaped control characters can exceed that limit, leaving the call to time out and tear down the helper; reject oversized serialized requests before writing.</violation>
</file>
<file name="apps/sim/app/o/[organizationId]/benchmark/components/benchmark-score-chart.tsx">
<violation number="1" location="apps/sim/app/o/[organizationId]/benchmark/components/benchmark-score-chart.tsx:40">
P2: The chart marks the selected run only visually, so screen-reader users cannot distinguish it from the other buttons. Add `aria-pressed` to expose the selected state.</violation>
</file>
<file name="apps/sim/lib/mothership/tools/client/computer-tool-execution.ts">
<violation number="1" location="apps/sim/lib/mothership/tools/client/computer-tool-execution.ts:182">
P2: Large but valid screenshots cannot reach the completion endpoint: an 8 MiB PNG base64-encodes above the 10 MiB JSON body cap, and retries leave the screenshot unchanged. Bound or omit oversized screenshots before reporting so the tool call can settle.</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.
Turn on auto-fix | Re-trigger cubic
| /> | ||
| ) : ( | ||
| <> | ||
| {benchmarks.error ? ( |
There was a problem hiding this comment.
P2: A failed “Load older benchmarks” request sets error but retains earlier pages, so this branch hides the list and retry button. Keep cached pages visible on next-page errors and let the button retry.
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/app/o/[organizationId]/benchmark/benchmark.tsx, line 68:
<comment>A failed “Load older benchmarks” request sets `error` but retains earlier pages, so this branch hides the list and retry button. Keep cached pages visible on next-page errors and let the button retry.</comment>
<file context>
@@ -0,0 +1,120 @@
+ />
+ ) : (
+ <>
+ {benchmarks.error ? (
+ <p role='alert' className='text-[var(--text-error)] text-small'>
+ {benchmarks.error.message}
</file context>
| {benchmarks.error ? ( | |
| {benchmarks.error && !benchmarks.isFetchNextPageError ? ( |
| this.stopWithError(new Error('Computer Use timed out; inspect the app before retrying.')) | ||
| }, REQUEST_TIMEOUT_MS) | ||
| this.pending.set(id, { resolve, reject, timer }) | ||
| child.stdin.write(`${JSON.stringify({ id, method, params })}\n`, (error) => { |
There was a problem hiding this comment.
P2: ComputerUse.swift rejects requests over 128 KiB under ID invalid-request, which this client ignores. Schema-valid text with enough escaped control characters can exceed that limit, leaving the call to time out and tear down the helper; reject oversized serialized requests before writing.
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/desktop/src/main/computer-use/native-client.ts, line 48:
<comment>`ComputerUse.swift` rejects requests over 128 KiB under ID `invalid-request`, which this client ignores. Schema-valid text with enough escaped control characters can exceed that limit, leaving the call to time out and tear down the helper; reject oversized serialized requests before writing.</comment>
<file context>
@@ -0,0 +1,142 @@
+ this.stopWithError(new Error('Computer Use timed out; inspect the app before retrying.'))
+ }, REQUEST_TIMEOUT_MS)
+ this.pending.set(id, { resolve, reject, timer })
+ child.stdin.write(`${JSON.stringify({ id, method, params })}\n`, (error) => {
+ if (error && this.child === child)
+ this.stopWithError(new Error('Computer Use helper disconnected.'))
</file context>
| : result.kind === 'action' && !result.verified | ||
| ? 'Input was dispatched. Inspect the returned observation or the app to verify its effect.' | ||
| : 'Computer observation completed', | ||
| data: computerToolResultForModel(result), |
There was a problem hiding this comment.
P2: Large but valid screenshots cannot reach the completion endpoint: an 8 MiB PNG base64-encodes above the 10 MiB JSON body cap, and retries leave the screenshot unchanged. Bound or omit oversized screenshots before reporting so the tool call can settle.
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/mothership/tools/client/computer-tool-execution.ts, line 182:
<comment>Large but valid screenshots cannot reach the completion endpoint: an 8 MiB PNG base64-encodes above the 10 MiB JSON body cap, and retries leave the screenshot unchanged. Bound or omit oversized screenshots before reporting so the tool call can settle.</comment>
<file context>
@@ -0,0 +1,211 @@
+ : result.kind === 'action' && !result.verified
+ ? 'Input was dispatched. Inspect the returned observation or the app to verify its effect.'
+ : 'Computer observation completed',
+ data: computerToolResultForModel(result),
+ }
+ } else
</file context>
| import { benchmarkOperations } from '@/lib/benchmarks/application/operations' | ||
| import { runBenchmarkComparison } from '@/lib/benchmarks/application/run-comparison' | ||
|
|
||
| export const POST = defineInternalJsonRoute({ |
There was a problem hiding this comment.
c88d1a8 to
468fc3f
Compare
|
@cubic-dev-ai review this PR |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
faff0a8 to
df7a02b
Compare
|
@cubic-dev-ai review this PR |
@icecrasher321 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 269 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
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.
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
This branch was successfully deployed
2 participants
Summary
Add private named knowledge graphs, detailed Plan-mode specifications and an organization benchmark console. Plan and knowledge use the same server-side eligibility as Benchmark UI: the existing benchmark deployment opt-in plus platform admin status and enabled Super User Mode. The UI, chat creation/admission, graph settings and worker memory scope enforce that policy; ordinary organization admin status is insufficient.
The branch also adds separately gated desktop computer use, read-only Search integration tooling, catalog credential/scope guidance and client-tool result recovery fixes. Computer use and Search tools retain their separate rollout gates; no production flag is enabled by this PR. A benchmark's selected user must independently qualify for Plan.
Type of Change
Testing
bun run test: 355 script tests and all 19 workspace tasks pass; Sim app 34,918 pass, 21 skip.Review fixes cover selected-user admission, lease renewal/expiry, timestamp precision, score constraints, draft preservation, scoped search selection, native replay/visual transport and memory binding on workspace transfer. Additive migrations
0395–0399introduce graph/benchmark storage and theNOT VALIDscore check. Dev databases retaining the earlier feature migration journal still require the documented reconciliation before redeployment; no live journal or configuration was changed.No customer content, paid comparison runs, deployment or merge. Earlier failed local receipts are retained: an outdated Plan-positive fixture needed explicit eligibility, and an initial heavily concurrent script run hit fixture timeouts. The corrected full rerun passes without weakening assertions or test deadlines.
Checklist
Screenshots/Videos
No new screenshots captured for the eligibility change. Plan and knowledge settings use the existing composer and settings layouts.
Companion PR
Companion: https://github.com/simstudioai/mothership/pull/598