Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR introduces three new read-only copilot tools (
Confidence Score: 4/5Safe to merge with the args nullability issue resolved — all other changes are additive registrations and a well-contained bug fix. The core logic (read-only guards, tool registration, schema sync, bug fix in terminal data) is sound and well-commented. One concern is the apps/sim/lib/copilot/tools/server/knowledge/search-knowledge-base.ts and apps/sim/lib/copilot/tools/server/table/query-user-table.ts — both type Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Copilot Model Request] --> B{Tool Route}
B -->|run_code| C[executeRunCode]
C --> D{outputs / outputTable in params?}
D -->|Yes| E[Return error: compute-only]
D -->|No| F[executeFunctionExecute sandbox execution]
B -->|search_knowledge_base| G[searchKnowledgeBaseServerTool]
G --> H{operation in READ_OPERATIONS?}
H -->|No| I[Return error: read-only]
H -->|Yes| J[knowledgeBaseServerTool.execute]
B -->|query_user_table| K[queryUserTableServerTool]
K --> L{operation in READ_OPERATIONS?}
L -->|No| M[Return error: read-only]
L -->|Yes| N{outputPath present?}
N -->|Yes| O[Return error: read-only]
N -->|No| P[userTableServerTool.execute]
B -->|search subagent| Q[Search Subagent replaces Research]
F & J & P & Q --> R[Tool Result]
R --> S[getToolCallTerminalData]
S --> T{failed AND output defined?}
T -->|Yes| U[Merge error into output for model context]
T -->|No| V[Return output or error as-is]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Copilot Model Request] --> B{Tool Route}
B -->|run_code| C[executeRunCode]
C --> D{outputs / outputTable in params?}
D -->|Yes| E[Return error: compute-only]
D -->|No| F[executeFunctionExecute sandbox execution]
B -->|search_knowledge_base| G[searchKnowledgeBaseServerTool]
G --> H{operation in READ_OPERATIONS?}
H -->|No| I[Return error: read-only]
H -->|Yes| J[knowledgeBaseServerTool.execute]
B -->|query_user_table| K[queryUserTableServerTool]
K --> L{operation in READ_OPERATIONS?}
L -->|No| M[Return error: read-only]
L -->|Yes| N{outputPath present?}
N -->|Yes| O[Return error: read-only]
N -->|No| P[userTableServerTool.execute]
B -->|search subagent| Q[Search Subagent replaces Research]
F & J & P & Q --> R[Tool Result]
R --> S[getToolCallTerminalData]
S --> T{failed AND output defined?}
T -->|Yes| U[Merge error into output for model context]
T -->|No| V[Return output or error as-is]
Reviews (1): Last reviewed commit: "fix(copilot): failed tool calls must sur..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (params && 'outputPath' in (params as Record<string, unknown>)) { | ||
| return { | ||
| success: false, | ||
| message: | ||
| 'query_user_table is read-only: outputPath (file export) is not available; digest the rows directly or route exports through the table agent', | ||
| } | ||
| } |
There was a problem hiding this comment.
outputPath check
The check at lines 35–40 already guards against outputPath nested inside params.args. The second check at lines 42–48 looks for outputPath at the top level of params, but outputPath is not a valid top-level field in QueryUserTableArgs — the catalog schema only defines operation and args at this level. If the Go executor or the server framework normalises the payload into the typed shape before reaching this handler, the top-level path can never be reached, making this check dead code. If the raw payload could arrive unnormalised, a note explaining that assumption would help clarify intent.
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!
| type SearchKnowledgeBaseArgs = { | ||
| operation: string | ||
| args?: Record<string, any> | ||
| } |
There was a problem hiding this comment.
args typed optional but required in schema
SearchKnowledgeBaseArgs.args is typed as optional (args?: Record<string, any>), yet the generated catalog entry at SearchKnowledgeBase.parameters.required includes 'args'. The same pattern exists in QueryUserTableArgs. If the LLM omits args, the handler forwards params with args: undefined to knowledgeBaseServerTool.execute, where downstream property accesses like params.args.knowledgeBaseId would throw. Aligning the TypeScript type with the schema (args: Record<string, any> without ?) makes the contract explicit and prevents a runtime crash on malformed calls.
PR SummaryMedium Risk Overview OAuth2 authorize now supports optional Mothership chat fork copies uploads born at or before the fork point: storage quota gate, physical blob copies with new ids/keys, transcript and resource reference rewrites, ghost resource cleanup, failed-copy row deletion +
Function execute returns 422 when sandbox mounts/exports are requested without E2B; overwrite exports get byte-compare unchanged warnings, size/sha256 receipts, and a leading newline on E2B result markers. Streamed workflow remove events evict workflows from the active React Query cache. Smaller changes: credential reconnect link labels, Reviewed by Cursor Bugbot for commit 07af501. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| answersByIndex[index] = answers | ||
| hiddenUserByIndex[index + 1] = true | ||
| } | ||
| return { answersByIndex, hiddenUserByIndex } |
There was a problem hiding this comment.
Last tag wins pairing
Medium Severity
When one assistant turn contains more than one question segment, pairing uses only the last tag’s schema via parseLastQuestionTag, but every QuestionDisplay on that turn receives the same questionAnswers. Earlier cards can show another batch’s prompts/answers or blank recap lines.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fcfacf9. Configure here.
| setStep(next) | ||
| const prefill = customByStep[next] ?? '' | ||
| setFreeText(prefill) | ||
| setFreeTextEditing(prefill.trim().length > 0) |
There was a problem hiding this comment.
Stepper ignores committed custom text
Medium Severity
In multi-step question cards, goToStep calls commitCustom() but reads the prefilled “Something else” value from stale customByStep instead of the array commitCustom returns. After moving forward and back, earlier typed custom answers can disappear from the input even though they were just committed.
Reviewed by Cursor Bugbot for commit 8b6c2df. Configure here.
| if (!lines[i].startsWith(prefix)) return null | ||
| answers.push(lines[i].slice(prefix.length)) | ||
| } | ||
| return answers |
There was a problem hiding this comment.
Multiline answers break pairing
Medium Severity
Question answers are serialized with newlines between questions, but each answer is not escaped. A multiline “Something else” value adds extra lines, so parseQuestionAnswerMessage treats them as more questions, pairing fails, the user bubble stays visible, and the card never shows the answered recap after reload.
Reviewed by Cursor Bugbot for commit 040dea9. Configure here.
| const { copied, failed, failedCopyIds } = await executeChatFileBlobCopies(result.blobTasks, { | ||
| userId, | ||
| workspaceId: parent.workspaceId ?? undefined, | ||
| }) |
There was a problem hiding this comment.
Quota checked before copy completes
Low Severity
Forking checks storage quota from planned byte totals, then commits the chat and copies blobs afterward. Concurrent forks or other uploads can consume quota between the check and incrementStorageUsage, so copies can succeed even when the workspace is already at or over its limit.
Reviewed by Cursor Bugbot for commit 040dea9. Configure here.
Stop the mothership from adopting a workspace user-skill on its own: - Remove the load_user_skill tool and its three payload callers (chat payload, mothership execute route, inbox executor); delete lib/mothership/skills.ts + its test. Skills no longer autoload as the agent's own instructions. - Rename the workspace "## Skills" inventory to "## Agent Block Skills — NOT FOR YOU" with a one-line guardrail so a skill's description (e.g. "respond like a pirate") is not treated as an instruction. Skills reach the model as behavior only via explicit /-attach. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 6 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 56c7af1. Configure here.
| ) | ||
| } | ||
|
|
||
| if (data.length === 0 || phase === 'dismissed') return null |
There was a problem hiding this comment.
Dismissed question card reappears
Medium Severity
Dismissing a question card only updates local React state (phase === 'dismissed'). After a reload or remount, that state resets to active, so the card shows again even though the user already dismissed it and may have continued the chat another way.
Reviewed by Cursor Bugbot for commit 56c7af1. Configure here.
…top it clearing them (#5546) * fix(copilot): persist KB tag subblocks as JSON strings from edit_workflow The edit_workflow tool normalizes array-with-id subblocks (via normalizeArrayWithIds) but only re-stringifies the keys listed in JSON_STRING_SUBBLOCK_KEYS. `tagFilters` (knowledge-tag-filters) and `documentTags` (document-tag-entry) were missing, so agent-authored tag filters were stored as raw JSON arrays while those UI components read their value with JSON.parse (expecting a string). The result: an agent edit to a Knowledge block's tag filter persisted correctly but rendered as an empty filter in the editor (JSON.parse on an array throws -> []). - Add `tagFilters` and `documentTags` to JSON_STRING_SUBBLOCK_KEYS so edit_workflow stores them in the same shape the UI writes. - Make both components' parsers tolerate an already-parsed array on read, self-healing values already persisted in the broken (array) shape. Search execution was unaffected (parseTagFilters accepts arrays), so the value was never lost — only the editor render and round-trip were broken. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(copilot): expose KB tag definitions in VFS meta.json Surface each knowledge base's defined tags (displayName -> tagSlot) inline in its meta.json via serializeKBMeta, loaded in one batched query (loadKbTagDefinitions), so the agent can bind a knowledge-tag filter to a real tag slot instead of guessing a tag name it cannot otherwise see. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(copilot): stringify KB tag subblocks on the nested-node edit path The nested-node merge path normalized array-with-id subblocks but never re-serialized the JSON_STRING_SUBBLOCK_KEYS, so editing a block nested in a loop/parallel container still persisted tagFilters/documentTags (and conditions/routes) as raw arrays -- the exact shape the subblock components cannot JSON.parse. Route all four write paths through a single normalizeSubblockValue helper so the normalize and re-stringify steps cannot drift apart again, and extract the duplicated string-or-array read logic into parseJsonArrayValue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(copilot): tighten subblock serialization helpers Derive KbTagDefinitionSummary from the canonical TagDefinition instead of restating its fields, make parseJsonArrayValue generic so callers drop their `as T[]` casts, and unexport the three builders helpers that no longer have consumers outside the module now that normalizeSubblockValue fronts them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(copilot): stop stripping tagFilters/documentTags from the agent's workflow view sanitizeForCopilot dropped `tagFilters` and `documentTags` from the workflow state the agent reads (workflows/{name}/state.json), while edit_workflow is allowed to write both. The field was therefore write-only: on a follow-up edit the agent read back an absent field, concluded no filter was set, and cleared the user's tag filter. The redaction was introduced for workflow *export* (#1628) and is already enforced there by sanitizeWorkflowForSharing's key list. The duplicate in the copilot-only sanitizeSubBlocks was redundant for export and destructive for the agent. Removes it and pins the contract with a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(copilot): reject malformed KB tag values instead of clearing the filter `knowledge-tag-filters` and `document-tag-entry` had no arm in the `edit_workflow` input validator, so they fell through to the pass-through default. Any non-array value the agent supplied -- a double-encoded JSON string, an object, an unparseable string -- reached `normalizeSubblockValue`, where `normalizeArrayWithIds` coerces unparseable input to `[]`. The write path then persisted `"[]"` over the tag filter the user had configured. `condition-input` and `router-input` already guard against exactly this and return an actionable error to the model. Extend that arm to cover the two KB subblock types. It keys on subblock type, so the unrelated `tagFilters` short-input on the Algolia block is unaffected. `null`/`undefined` and empty arrays still clear the field, so intentional clears keep working. Also wrap `loadKbTagDefinitions` in try/catch. Tag definitions are an optional meta.json enrichment, but the query ran inside the top-level `Promise.all`, so a transient failure would reject the entire workspace VFS materialize and leave the agent unable to read any file. Now it degrades to a meta.json without tag definitions, matching the sibling materializers. Adds regression tests for both, plus the first tests for `parseJsonArrayValue`, the helper that keeps pre-fix raw-array rows readable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(copilot): collapse duplicate JSON-array parsing in edit-workflow builders `normalizeArrayWithIds` and `normalizeConditionRouterIds` each hand-rolled the same "accept a raw array or the JSON string these subblocks persist" parse. Extract `parseJsonArray`, which returns null when the value is neither, so each caller keeps its own distinct fallback: `[]` for the former, the untouched original value for the latter. Behavior-preserving. An empty array is truthy, so `[]` and `"[]"` still parse through rather than hitting either fallback. `validation.ts` has a third copy, but `builders.ts` already imports from it, so sharing the helper across the two would introduce an import cycle. Left as is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(copilot): specify tag name and legal operators in KB meta.json `tagDefinitions` exposed `displayName`, but a `tagFilters` entry must carry the key `tagName`. An entry written with `displayName` passes validation and persists, then filters nothing -- a silent failure. Rename the field at the serializer boundary; the DB column is untouched. Also emit the operators legal for each tag's `fieldType`, reusing `getOperatorsForFieldType`. `between` is valid for number and date but not for text or boolean, and the agent has no way to infer that. An unrecognized fieldType yields an empty list rather than throwing. Still unspecified, and deliberately out of scope: a filter entry's value key is `tagValue` (but `value` on documentTags), and `between` needs `valueTo`. Those describe the subblock entry shape, not the knowledge base, so meta.json is the wrong place for them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(copilot): pass a nullish subblock clear through instead of serializing "[]" `validateValueForSubBlockType` accepts null as an explicit clear, but `normalizeSubblockValue` then ran it through `normalizeArrayWithIds`, which coerces any non-array to `[]`, and persisted the string "[]". No data is lost either way -- "[]" and an absent field both mean "no filters". But it left the field present when the caller asked for it to be unset, so `sanitizeForCopilot` showed the agent an empty filter rather than an absent one, contradicting the absent-means-unset invariant the sanitizer documents. It also made Algolia's `if (params.tagFilters)` see a set value, since "[]" is truthy. An explicitly empty array still serializes to "[]" -- clearing with a value is distinct from clearing by omission. Reported by Cursor Bugbot on #5546. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 participants
Summary
Brief description of what this PR does and why.
Fixes #(issue)
Type of Change
Testing
How has this been tested? What should reviewers focus on?
Checklist
Screenshots/Videos