Merge remote-tracking branch 'origin/fix/w6-tidy' into integrate/v2-w5

Author: waleedlatif1

apps/sim/app/api/v2/workflows/[id]/deployment/route.ts

@@ -21,11 +21,9 @@ export const revalidate = 0
  * fallback: it retains the timestamp of a deployment that has since been
  * undeployed, so reading it would report a deploy time alongside
  * `isDeployed: false`.
- */
-/**
- * Deliberately head-safe despite issuing a write.
  *
- * Reading a workflow can trigger a migrate-on-read `workflow_blocks` update when
+ * Deliberately head-safe despite issuing a write. Reading a workflow can trigger
+ * a migrate-on-read `workflow_blocks` update when
  * `applyBlockMigrations` upgrades a stored block. That write is convergent: it is
  * conditional on a migration actually applying, idempotent, and would be issued by
  * the next ordinary read regardless, so a `HEAD` only brings it forward.

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/utils.test.ts

@@ -2,6 +2,7 @@
  * @vitest-environment node
  */
 import { describe, expect, it } from 'vitest'
+import { columnTypeOf } from '@/lib/table/column-types'
 import {
   cleanCellValue,
   dateValueToLocalParts,
@@ -160,6 +161,32 @@ describe('cleanCellValue', () => {
     expect(cleanCellValue('Nope', column)).toBeNull()
     expect(cleanCellValue('Bug, Nope', column)).toBeNull()
   })
+
+  /**
+   * The refusal above is `coerce`'s, not the last word the registry has on the
+   * value: `salvage` reads the same paste as the one option that resolved. That
+   * reading is reserved for writes with no caller to answer — a CSV row, a block
+   * output — and a typed cell has one, so this helper must not reach for it. The
+   * pairing is asserted rather than described so a future helper that "improves"
+   * the paste by salvaging it fails here.
+   */
+  it('refuses a partial multiselect paste the registry could still salvage', () => {
+    const column = {
+      name: 'tags',
+      type: 'select',
+      multiple: true,
+      options: [
+        { id: 'opt_a', name: 'Bug' },
+        { id: 'opt_b', name: 'Docs' },
+      ],
+    } as const
+
+    expect(columnTypeOf(column).salvage?.('Bug, Nope', column)).toEqual({
+      ok: true,
+      value: ['opt_a'],
+    })
+    expect(cleanCellValue('Bug, Nope', column)).toBeNull()
+  })
 })
 
 describe('formatValueForInput', () => {

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/utils.ts

@@ -19,8 +19,18 @@ export function generateColumnName(columns: ReadonlyArray<{ name: string }>): st
 }
 
 /**
- * Coerce a raw input value to the appropriate type for a column.
- * Throws on invalid JSON.
+ * Coerce a value a person typed or pasted into a cell to that column's type.
+ * Throws on invalid JSON, and answers `null` for everything else the column
+ * type refuses.
+ *
+ * `null` is what the server would store for the same value, which is the point:
+ * the optimistic cache and the row that comes back agree. It deliberately does
+ * not consult `ColumnTypeDefinition.salvage`, which reads a refused value
+ * lossily — a multiselect paste naming one option that no longer exists blanks
+ * the cell here rather than storing the members that did resolve. Salvage is
+ * reserved for writes with no caller to answer, and this one has one: a person
+ * watching the cell, who is better served seeing the paste refused than seeing
+ * part of it silently kept.
  */
 export function cleanCellValue(
   value: unknown,

apps/sim/hooks/queries/mcp.ts

@@ -317,6 +317,14 @@ export function useCreateMcpServer() {
         authType,
       }
     },
+    /**
+     * Both caches are dropped, so neither waits out its stale time — but the
+     * refetched row still reads `disconnected`, because the discovery that
+     * moves it runs on the tools query this same invalidation kicks off, after
+     * the list has already come back. The status catches up on the next list
+     * refetch; the tools do not wait for it, since
+     * {@link isServerEligibleForDiscovery} gates only OAuth rows on `connected`.
+     */
     onSettled: (_data, _error, variables) => {
       queryClient.invalidateQueries({ queryKey: mcpKeys.serversList(variables.workspaceId) })
       queryClient.invalidateQueries({

apps/sim/lib/api/contracts/v2/openapi/shared.ts

@@ -257,6 +257,14 @@ export const WORKSPACE_API_KEY_DENIED =
  * {@link WORKSPACE_API_KEY_DENIED} for an operation behind the resource-concealment
  * error policy, which rewrites the authorization failure to a not-found response so
  * the caller learns nothing about the resource.
+ *
+ * Published on no operation today: every one audited so far refuses a workspace
+ * key through its principal-kind list, which raises an error the concealment
+ * policy does not rewrite, so all of them say 403. Kept because a concealed
+ * operation that denies the key through the policy itself would need this exact
+ * wording, and because `scripts/openapi/documents.test.ts` asserts the file-share
+ * description does not carry it — inlining the string there would let the guard
+ * and the wording it guards drift apart.
  */
 export const WORKSPACE_API_KEY_DENIED_AS_NOT_FOUND =
   'A workspace API key is rejected as `404` rather than `403`, because unauthorized resources are concealed; use a personal API key.'

apps/sim/lib/api/contracts/v2/workflows.ts

@@ -816,18 +816,15 @@ export const v2ExecutionErrorSchema = z
 export type V2ExecutionError = z.output<typeof v2ExecutionErrorSchema>
 
 /**
- * The mutually-exclusive execute option matrix, mirrored from the route's
- * post-parse checks in `app/api/v2/workflows/[id]/execute/route.ts`. Kept as one
+ * That the execute options constrain each other, said once. Kept as one
  * exported string so the request-body description and the OpenAPI operation
  * description cannot drift from each other.
- */
-/**
- * The six rejected option combinations used to be enumerated here and pasted
- * onto both the operation and the request-body description, restating what each
- * field already says. A caller reads the constraint where it applies — on the
- * field it constrains — so the enumeration lives on `async`, `stream`,
- * `executionTimeoutSeconds`, `includeThinking`, and `includeToolCalls`, and the
- * operation says only that the options are mutually constrained.
+ *
+ * It deliberately does not enumerate the combinations the route rejects. That
+ * list used to be pasted onto both the operation and the request-body
+ * description, restating what each field already says; a caller reads a
+ * constraint where it applies, so it lives on `async`, `stream`,
+ * `executionTimeoutSeconds`, `includeThinking`, and `includeToolCalls`.
  */
 export const EXECUTE_OPTION_CONSTRAINTS =
   'Each option carries the modes it requires and the modes that reject it; a violated combination is a 400.'
@@ -838,8 +835,9 @@ export const EXECUTE_OPTION_CONSTRAINTS =
  * (triggerType, draft state, deployment pinning) are NEVER wire fields; they
  * are typed options on the execution service.
  *
- * The six rejected option combinations are enumerated in
- * {@link EXECUTE_OPTION_CONSTRAINTS} and enforced by the route after parsing.
+ * The rejected option combinations are enforced by the route after parsing and
+ * described on the fields they constrain; {@link EXECUTE_OPTION_CONSTRAINTS}
+ * only tells a caller that the options constrain each other.
  */
 export const v2ExecuteWorkflowBodySchema = z
   .object({

apps/sim/lib/api/list-query.ts

@@ -57,17 +57,6 @@ export type CursorKey = string | number
 export const INVALID_CURSOR_MESSAGE =
   'cursor does not match the requested sortBy/sortOrder. Restart pagination without a cursor after changing the sort.'
 
-/**
- * Caller-facing message for a cursor that cannot be read back at all.
- *
- * Separate from {@link INVALID_CURSOR_MESSAGE} because that one names
- * `sortBy`/`sortOrder`, and the lists that mint a wrapped domain token accept
- * neither param — `GET /logs` carries its direction in `order`, and
- * `GET /billing/logs` takes no sort param whatsoever. Sending those callers to
- * inspect a knob their operation does not have is the same wrong-signpost
- * problem `UNKNOWN_CURSOR_MESSAGE` was written to avoid on the ledger. The
- * actionable half — restart without a cursor — is identical.
- */
 /**
  * One column of a keyset ordering, with the codec that moves its value through
  * the opaque cursor.

apps/sim/lib/credentials/application/list-workspace-credentials.ts

@@ -25,12 +25,6 @@ export interface ListWorkspaceCredentialsInput {
 export interface ListWorkspaceCredentialsResult {
   credentials: VisibleWorkspaceCredential[]
   nextCursorKeys: CursorKey[] | null
-  /**
-   * Echoed back because the route's presenter receives only this result, and the
-   * cursor it hands out has to be stamped with the sort that produced it.
-   */
-  sortBy: ListWorkspaceCredentialsInput['sortBy']
-  sortOrder: ListSortOrder
 }
 
 export const listWorkspaceCredentials = defineAuthorizedWorkspaceUseCase({
@@ -57,7 +51,7 @@ export const listWorkspaceCredentials = defineAuthorizedWorkspaceUseCase({
         limit: input.limit,
         cursorKeys: input.cursorKeys,
       })
-      return { credentials: page.data, nextCursorKeys: page.nextCursorKeys, ...sort }
+      return { credentials: page.data, nextCursorKeys: page.nextCursorKeys }
     }
 
     const workspaceAccess = await checkWorkspaceAccess(context.workspaceId, principal.userId)
@@ -82,6 +76,6 @@ export const listWorkspaceCredentials = defineAuthorizedWorkspaceUseCase({
       limit: input.limit,
       cursorKeys: input.cursorKeys,
     })
-    return { credentials: page.data, nextCursorKeys: page.nextCursorKeys, ...sort }
+    return { credentials: page.data, nextCursorKeys: page.nextCursorKeys }
   },
 })

apps/sim/lib/knowledge/application/search.test.ts

@@ -352,6 +352,22 @@ describe('knowledge search application use case', () => {
       expect(result.results[0]).not.toHaveProperty('rerankerScore')
     })
 
+    /**
+     * A resolved call with an empty ordering leaves the caller in the same place a
+     * thrown one does — vector order, no `rerankerScore` — so it reports the same
+     * status. It is not "the reranker matched nothing": `rerank` sends a non-empty
+     * document list and asks for `top_n` of it, so an empty array means the
+     * response carried nothing usable rather than a legitimate empty ranking.
+     */
+    it('reports unavailable when the call resolves without a usable ordering', async () => {
+      mocks.rerank.mockResolvedValueOnce({ results: [], isBYOK: false })
+
+      const result = await rerankedSearch(true)
+
+      expect(result.rerankerStatus).toBe('unavailable')
+      expect(result.results[0]).not.toHaveProperty('rerankerScore')
+    })
+
     it('reports skipped for a tag-only search, which has no query to rank against', async () => {
       mocks.getTagDefinitions.mockResolvedValue([
         { tagSlot: 'tag1', displayName: 'team', fieldType: 'text' },

apps/sim/lib/knowledge/application/search.ts

@@ -328,8 +328,18 @@ export const searchKnowledge = defineAuthorizedKnowledgeUseCase({
      * never completes, so only the success path has to move it. A request with
      * nothing to rank — no query text, or no candidate rows — is `skipped` rather
      * than `unavailable`: the reranker was never the obstacle. Anything else that
-     * was asked for and did not run is `unavailable`, including a request that
-     * reaches here with no model, which no HTTP contract can now produce.
+     * was asked for and did not produce a usable ordering is `unavailable`,
+     * including a request that reaches here with no model, which no HTTP contract
+     * can now produce.
+     *
+     * A call that returns without raising but hands back an empty ordering counts
+     * as `unavailable` too, and it is not the reranker "matching nothing":
+     * `rerank` asks for `top_n` over a non-empty document list, so a provider that
+     * ranked them returns one entry per document. Empty means the response carried
+     * nothing usable — no results, or only indices outside the batch, which
+     * `rerank` drops. The caller is left in vector order with no `rerankerScore`,
+     * which is exactly what `unavailable` promises, and retrying is exactly the
+     * right advice.
      */
     let rerankerStatus: RerankerStatus = !input.rerankerEnabled
       ? 'not_requested'