fix(copilot): standardize entity tool contracts and review hydration#139
Merged
Conversation
…138) * feat(copilot): standardize entity-scoped tool contracts Unify workflow and custom tool request shapes around entity ids/documents, tighten custom tool schema validation, and thread the new provenance through client and server tool execution.\n\nCo-authored-by: Codex <codex@openai.com> * feat(copilot): persist staged tool review state Add reusable staged-review handling for client tools, hydrate plan todos from persisted messages, and carry tool result data through the streaming UI. Co-authored-by: Codex <codex@openai.com> * fix(i18n): update waitlist text and format JSON for consistency * feat(copilot): enhance tool validation descriptions and auto-execute eligible tools based on access level * fix(copilot): enhance tool validation and auto-execution logic for access levels * fix(copilot): streamline plan todos hydration by filtering for successful tool calls * fix(copilot): refactor tool execution logic and enhance message handling for pending tools * fix(copilot): enhance tool call handling by ensuring unique tool call IDs and updating test cases for accuracy --------- Co-authored-by: Codex <codex@openai.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
| Filename | Overview |
|---|---|
| apps/tradinggoose/stores/copilot/tool-registry.ts | Adds handleCopilotServerToolSuccess helper and moves environment-store reload into it; renames provenance fields from workflowId/contextWorkflowId to contextEntityKind/contextEntityId; prepareCopilotToolArgs now strict-parses args through Zod schemas, which can reject old-format args from in-flight sessions |
| apps/tradinggoose/stores/copilot/store-messages.ts | Drops legacy toolCalls array processing from normalizeMessagesForUI, buildPinnedToolCallsById, updateMessagesForToolCallState, and validateMessagesForLLM; adds buildPlanTodosFromMessages to rebuild plan todo state from persisted contentBlocks-format tool results using canonical id field |
| apps/tradinggoose/stores/copilot/store-provenance.ts | Migrates provenance from contextWorkflowId/workflowId to contextEntityKind/contextEntityId; non-workflow reviewTarget no longer propagates entityId, reviewSessionId, or draftSessionId into provenance — only workspaceId is carried forward; removes toolCalls-based lookup from findAssistantMessageIdForToolCall |
| apps/tradinggoose/stores/copilot/store.ts | Reorders stream-completion flow so resolveFinalStreamTurnState and abort-controller cleanup run after flushPendingAutoExecutionToolCalls; removes review state from access-level auto-execution; wires buildPlanTodoStateFromMessages into all chat-load and sync paths |
| apps/tradinggoose/stores/copilot/streaming.ts | Converts auto-tool execution from fire-and-forget setTimeout to sequential await; pipes tool result through buildStreamedToolDisplayState so result data is visible in streaming UI state; awaiting_tools handler now sets latestTurnStatus = ACTIVE_TURN_STATUS and awaitingTools = true |
| apps/tradinggoose/lib/copilot/tools/client/base-tool.ts | Extracts StagedReviewClientTool abstract base class with stageReviewResult, getStagedReviewResult, and prepareReviewAccept lifecycle; removes per-tool lastResult fields and getInterruptDisplays overrides |
| apps/tradinggoose/lib/copilot/tools/entity-target.ts | New shared helper for canonical entityId / contextEntityKind resolution across client and server tools |
| apps/tradinggoose/lib/copilot/tools/server/workflow/workflow-scope.ts | New helper centralizing workflow-scoped server access: resolveServerWorkflowScope verifies access via verifyWorkflowAccess; resolveServerWorkspaceId falls back to context.workspaceId when no workflow scope is resolved |
| apps/tradinggoose/app/api/copilot/execute-copilot-server-tool/route.ts | Adds explicit workspace membership check when context.workspaceId is present; schema extended with contextEntityKind, contextEntityId, workspaceId |
| apps/tradinggoose/app/api/copilot/chat/route.ts | Removes toolCalls from persistence path; converts LLM responseData.toolCalls to contentBlocks format on the non-SSE persist path, completing the migration away from the legacy column |
| packages/db/schema/copilot.ts | Removes tool_calls column from the Drizzle ORM model (soft deprecation — no DDL migration needed since the column has a default and existing data is simply abandoned) |
| apps/tradinggoose/lib/custom-tools/schema.ts | New Zod schemas (CustomToolParametersSchema, CustomToolOpenAiSchema, CustomToolUpsertRequestSchema) centralizing custom-tool document validation; introduces parseCustomToolSchemaText replacing the inline JSON.parse path |
| apps/tradinggoose/lib/copilot/runtime-tool-manifest-enrichment.ts | Simplifies semantic validators by inlining JSON-schema constants for TG_WORKFLOW, TG_BLOCK, and TG_EDGE metadata; removes AnnotatedGraphDocumentContract / string_document_contract path and the dynamic buildWorkflowEmbeddedDocumentValidators async import; switches preferredDocumentField from workflowDocument to entityDocument |
| apps/tradinggoose/lib/copilot/inline-tool-call.tsx | Review payload now visible in both review and success states; readEntityReviewPayload guards on result.entityKind presence instead of a hardcoded tool-name list; title changes to "Applied … Changes" in success state |
Sequence Diagram
sequenceDiagram
participant LLM
participant StreamHandler as SSE StreamHandler
participant ToolRegistry as prepareCopilotToolArgs (Zod parse)
participant ClientTool as StagedReviewClientTool
participant ServerRoute as /execute-copilot-server-tool
participant WorkflowScope as resolveServerWorkflowScope
participant DB
LLM->>StreamHandler: "tool_call {entityId, entityDocument}"
StreamHandler->>StreamHandler: applyStreamedFunctionCallItem
StreamHandler->>StreamHandler: stream_complete
StreamHandler->>ToolRegistry: executeCopilotToolCall
ToolRegistry->>ToolRegistry: ToolArgSchemas[name].parse(args) strict
ToolRegistry->>ClientTool: execute(args)
ClientTool->>ServerRoute: "executeCopilotServerTool({entityId, workspaceId})"
ServerRoute->>ServerRoute: checkWorkspaceAccess(workspaceId, userId)
ServerRoute->>WorkflowScope: resolveServerWorkflowScope(params, context)
WorkflowScope->>WorkflowScope: verifyWorkflowAccess(userId, entityId)
WorkflowScope-->>ServerRoute: "{workflowId, workspaceId, hasAccess}"
ServerRoute-->>ClientTool: staged result
ClientTool->>ClientTool: stageReviewResult(result) setState(review)
ClientTool-->>StreamHandler: awaited
StreamHandler->>StreamHandler: resolveFinalStreamTurnState
StreamHandler->>DB: persistChatMessages(contentBlocks)
DB-->>StreamHandler: ok
Note over StreamHandler: On reload: buildPlanTodosFromMessages(contentBlocks)
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/tradinggoose/stores/copilot/tool-registry.ts:275-288
**Strict Zod parse rejects in-flight session args after deployment**
`prepareCopilotToolArgs` now calls `ToolArgSchemas[toolName].parse(clonedArgs)` for every known tool. Schemas for `edit_workflow`, `rename_workflow`, `deploy_workflow`, `run_workflow`, `check_deployment_status`, `read_workflow`, `read_workflow_variables`, `set_workflow_variables`, and `read_workflow_logs` are all `.strict()` and now require `entityId` / `entityDocument` (not `workflowId` / `workflowDocument`). Any Copilot session that was opened before this deployment — where the LLM received the old manifest — will generate args containing `workflowId` / `workflowDocument`. The strict parse will throw a `ZodError`, the tool execution will surface as a client-side error, and the user must start a fresh session to recover. The old path returned `cloneArgs(args)` unconditionally, so this is a new failure mode with no graceful degradation.
Reviews (2): Last reviewed commit: "fix(copilot): normalize tool parameter h..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
292
to
293
| throw new Error('No active workspace found') | ||
| } |
Contributor
There was a problem hiding this comment.
resolveWorkspaceIdFromExecutionContext no longer has any fallback
The removed workflowId registry lookup was the only fallback for contexts where workspaceId wasn't set explicitly in the execution context. Now if executionContext.workspaceId is absent, the function throws immediately. This regresses any entity tool call (create/edit skill, indicator, etc.) that arrives from a Copilot session where workspaceId wasn't forwarded into provenance — for example, edge cases where the reviewTarget had no workspaceId or where buildTurnProvenanceFromContexts returned undefined (see separate comment on store-provenance.ts). Adding even a simple error context to the thrown error would make diagnosing this in production much easier.
Suggested change
| throw new Error('No active workspace found') | |
| } | |
| throw new Error( | |
| 'No active workspace found in execution context. Ensure workspaceId is included in tool provenance.' | |
| ) | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/tradinggoose/lib/copilot/tools/client/entities/entity-document-tool-utils.ts
Line: 292-293
Comment:
**`resolveWorkspaceIdFromExecutionContext` no longer has any fallback**
The removed `workflowId` registry lookup was the only fallback for contexts where `workspaceId` wasn't set explicitly in the execution context. Now if `executionContext.workspaceId` is absent, the function throws immediately. This regresses any entity tool call (create/edit skill, indicator, etc.) that arrives from a Copilot session where `workspaceId` wasn't forwarded into provenance — for example, edge cases where the `reviewTarget` had no `workspaceId` or where `buildTurnProvenanceFromContexts` returned `undefined` (see separate comment on `store-provenance.ts`). Adding even a simple error context to the thrown error would make diagnosing this in production much easier.
```suggestion
throw new Error(
'No active workspace found in execution context. Ensure workspaceId is included in tool provenance.'
)
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+6
to
+10
| export const CustomToolParametersSchema = z.object({ | ||
| type: z.literal('object'), | ||
| properties: z.record(z.any()), | ||
| required: z.array(z.string()).optional(), | ||
| }) |
Contributor
There was a problem hiding this comment.
CustomToolParametersSchema tightens type to z.literal('object') which could reject existing persisted schemas
The old schema accepted any string for parameters.type. The new CustomToolUpsertRequestSchema (consumed by the POST /api/tools/custom endpoint) and parseCustomToolSchemaText (used when Copilot submits a custom tool document) now both require type: 'object' exactly. Any existing workspace with a custom tool whose stored schema has parameters.type set to a different value (e.g. previously created through a less-strict code path) will receive a Zod parse error on the next Copilot edit or API upsert, silently blocking that tool from being updated.
Suggested change
| export const CustomToolParametersSchema = z.object({ | |
| type: z.literal('object'), | |
| properties: z.record(z.any()), | |
| required: z.array(z.string()).optional(), | |
| }) | |
| export const CustomToolParametersSchema = z.object({ | |
| type: z.union([z.literal('object'), z.string()]).transform(() => 'object' as const), | |
| properties: z.record(z.any()), | |
| required: z.array(z.string()).optional(), | |
| }) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/tradinggoose/lib/custom-tools/schema.ts
Line: 6-10
Comment:
**`CustomToolParametersSchema` tightens `type` to `z.literal('object')` which could reject existing persisted schemas**
The old schema accepted any string for `parameters.type`. The new `CustomToolUpsertRequestSchema` (consumed by the POST `/api/tools/custom` endpoint) and `parseCustomToolSchemaText` (used when Copilot submits a custom tool document) now both require `type: 'object'` exactly. Any existing workspace with a custom tool whose stored schema has `parameters.type` set to a different value (e.g. previously created through a less-strict code path) will receive a Zod parse error on the next Copilot edit or API upsert, silently blocking that tool from being updated.
```suggestion
export const CustomToolParametersSchema = z.object({
type: z.union([z.literal('object'), z.string()]).transform(() => 'object' as const),
properties: z.record(z.any()),
required: z.array(z.string()).optional(),
})
```
How can I resolve this? If you propose a fix, please make it concise.* fix(copilot): refine tool parameter handling and enhance error messages * fix(copilot): remove obsolete custom tool schema tests and update streaming handlers * refactor(copilot): remove legacy assistant toolCalls state Migrate Copilot message persistence and rendering to content blocks. Co-authored-by: Codex <codex@openai.com> * fix(copilot): propagate workspace context through server tools Validate server tool arguments and carry workspace context through Copilot execution. Co-authored-by: Codex <codex@openai.com> * refactor(copilot): simplify workflow document validators Remove the obsolete workflow document contract from Copilot manifest enrichment. Co-authored-by: Codex <codex@openai.com> * fix(copilot): update streaming logic and refine tool execution handling * fix(copilot): standardize tool argument handling and remove legacy toolCalls schema * fix(copilot): enhance tool argument handling and implement workspace access checks * fix(copilot): update todo execution state handling in tests and state builder * fix(copilot): update showPlanTodos logic to reflect only incomplete todos --------- Co-authored-by: Codex <codex@openai.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Promotes the current
stagingbranch intoupstream/main.entityIdandentityDocumentacross client tools, server tools, prompt metadata, manifests, execution context, and tests.Why
This removes workflow-specific Copilot boundary contracts, reduces schema drift between client/server/custom tool paths, and makes staged tool review behavior durable across hydration and reloads.
Affected Areas
apps/tradinggooseapps/docspackages/*Issue Links( if any )
Refs #138
Validation
Risk / Rollout Notes
Moderate Copilot risk because tool arguments and persisted execution provenance move from workflow-specific fields to entity-scoped fields. Key paths to watch after rollout: Copilot workflow edits, workflow runs/deployments, staged review accept/reject flows, custom tool import/upsert, and Google Drive/workspace-scoped server tools.
Backout: revert the merge commit from
main, or revert commit2777a382if applied directly.Config / Data Changes
entityIdor context before listing/reading resources.Screenshots / Video
Not captured. The visible changes are limited to Copilot inline tool review state rendering and waitlist copy; no broad layout changes are included.
Checklist