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 Introduces inline OAuth2 authorize gains optional Sim API key handling shifts to value-less Reviewed by Cursor Bugbot for commit ea7c26d. 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.
…KB tool handlers Mirrors mothership dev f90f9b05: - regenerated tool-catalog/tool-schemas mirrors (search trigger replaces research + scout; QueryUserTable / SearchKnowledgeBase entries) - queryUserTableServerTool / searchKnowledgeBaseServerTool: read-only wrappers delegating to the full user_table / knowledge_base handlers with hard operation allowlists (and outputPath export rejection on query_user_table) - display maps: 'search' agent label/title/icon added; research + scout entries retained so historical transcripts keep rendering - Search.id replaces Research.id in LONG_RUNNING_TOOL_IDS (it inherits research's long crawls) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mirrors mothership dev db60da94: run_code is the compute-only variant of function_execute for the search agent — same sandbox and inputs, no outputs.files / outputTable, so it cannot create or overwrite workspace resources. Wrapper handler hard-rejects the write vectors and delegates to executeFunctionExecute; run_code is deliberately absent from OUTPUT_PATH_TOOLS and the table output post-processor, so the name gating blocks writes even for leaked args. Added to LONG_RUNNING_TOOL_IDS, display title/icon maps, and the regenerated catalog/schema mirrors. Also removes two ineffective biome suppression comments in the docs workflow-preview (the rule doesn't fire in the docs app config). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…data
A failed handler result that carried a defined-but-empty output (the
app-tool executor's 'Tool not found' ships output: {}) won the priority
race in getToolCallTerminalData, so the resume payload's data — the only
thing the model reads — was a bare {} with the error text dropped. The
search agent retried run_code 20+ times blind against a stale server
because every failure rendered as empty instead of 'Tool not found'.
Failed calls now always carry error in their terminal data: merged into
object outputs, wrapped alongside non-object outputs, preserved when the
output already has an error field.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…splay Companion to mothership 8ae32e97 (user_memory tool removed — the feature no longer exists). Regenerates the mothership contract mirrors via generate-mship-contracts.ts, which also picks up the pending telemetry contract additions (gen_ai.agent.name labels, llm.client.context_tokens, llm.client.compactions, llm.request.compaction_trigger, llm.compaction.pause, gen_ai.usage.context_tokens), and removes the user_memory display title. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…select types only
UI ordering: answering a question card no longer echoes a duplicate user
bubble. The combined answer still goes on the wire as a user message, but the
chat pairs it back to its card (strict 'Prompt — Answer' match, now uniform
for single questions too) and renders the card as the answered recap — the
card IS the user turn, and the next assistant message streams below it. The
pairing is derived from the transcript, so live and reloaded renders are
identical; a dismissed card followed by an unrelated typed message does not
match and renders normally. Messages ending with a question card also drop
the copy/thumbs actions row — the card is an input surface, not a reactable
assistant turn.
Question types are now single_select and multi_select only: text is removed
(the free-text 'Something else' row covers it) and confirm collapses into
single_select with Yes/No options. multi_select rows toggle with a check and
the free-text row's arrow submits the step; answers are comma-joined labels
plus any typed entry. Agent-supplied catch-all options ('Other', 'Something
else', 'None of the above') are stripped at parse — the card always provides
its own free-text row; a question left with no real options is invalid.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Removes multi_select (and its toggle/check UI). The card is one shape: pick one option or type into the always-present 'Something else' row. Catch-all stripping and the transcript pairing/recap behavior are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Re-adds multi_select with a reworked interaction: option rows carry real checkboxes (emcn Checkbox chrome) instead of numbers and arrows, an option-styled Submit row confirms the step, and the "Something else" row reads as a plain option until clicked — then it becomes the focused text box, auto-checks, and can be unchecked without losing the typed text (blur with nothing typed reverts it). single_select behavior, catch-all stripping, and the transcript pairing/recap format are unchanged; multi_select answers are the checked labels comma-joined.
* feat(credentials): agent-initiated oauth credential reconnect * fix(credentials): address reconnect review findings * improvement(credentials): log when connect draft name lookups degrade
…flow 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>
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>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 5 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 ea7c26d. Configure here.
| const forkedMessages = messages.slice(0, forkIdx + 1) | ||
|
|
||
| // Resources are stored as a jsonb array on the chat row — copy them directly. | ||
| // Single workspace_files read per fork: every chat-owned upload. The |
There was a problem hiding this comment.
Fork drops paired question answers
Medium Severity
Branching at an assistant turn that shows an answered question recap only copies messages through that assistant row. The formatted answer lives in the immediately following user message, which the UI hides via question pairing, so the fork omits it and the new chat renders the card as unanswered.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ea7c26d. Configure here.
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