refactor(tools): execute internal operations in process - #7179
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Too many files changed for review (2480 files, 500 file limit). |
418c975 to
bb218f4
Compare
|
@cubic review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
![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 2480 files
Not reviewed (too large): apps/sim/tools/index.test.ts (~3,785 lines), apps/sim/lib/function-execution/execute-request.test.ts (~3,224 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
bb218f4 to
3d5e2dc
Compare
|
@cubic review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 2526 files
Not reviewed (too large): apps/sim/tools/index.test.ts (~3,790 lines), apps/sim/lib/function-execution/execute-request.test.ts (~3,224 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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
ae9c27f to
c8ee2b3
Compare
|
@cubic review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 2528 files
Confidence score: 4/5
- In
apps/sim/lib/internal/discord/operations.ts,textMessagecan report success when cancellation arrives aftersendDiscordMessagecompletes, unlike the multipart path; add the post-sendsignalcheck to keep cancellation behavior consistent.
Not reviewed (too large): apps/sim/tools/index.test.ts (~3,790 lines), apps/sim/lib/function-execution/execute-request.test.ts (~3,224 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/internal/discord/operations.ts">
<violation number="1" location="apps/sim/lib/internal/discord/operations.ts:36">
P2: When cancellation arrives after a text-only Discord request completes, `textMessage` returns success without the post-send cancellation check used by the multipart path. Check `signal` after `sendDiscordMessage` resolves so canceled executions do not report success.</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
c8ee2b3 to
5e6f76e
Compare
|
@cubic review |
@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 2528 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Not reviewed (too large): apps/sim/tools/index.test.ts (~3,790 lines), apps/sim/lib/function-execution/execute-request.test.ts (~3,224 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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.
Re-trigger cubic
5e6f76e to
097a27a
Compare
|
@cubic review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 2530 files
Not reviewed (too large): apps/sim/tools/index.test.ts (~3,790 lines), apps/sim/lib/function-execution/execute-request.test.ts (~3,224 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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.
Re-trigger cubic
097a27a to
8ec0260
Compare
|
@cubic review |
@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 2530 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Not reviewed (too large): apps/sim/tools/index.test.ts (~3,818 lines), apps/sim/lib/function-execution/execute-request.test.ts (~3,224 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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.
Re-trigger cubic
8ec0260 to
55e092a
Compare
|
@cubic review |
@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 2530 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Not reviewed (too large): apps/sim/tools/index.test.ts (~3,818 lines), apps/sim/lib/function-execution/execute-request.test.ts (~3,224 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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.
Re-trigger cubic
55e092a to
a907268
Compare
|
@cubic review |
@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 2530 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Not reviewed (too large): apps/sim/tools/index.test.ts (~3,818 lines), apps/sim/lib/function-execution/execute-request.test.ts (~3,224 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
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.
Re-trigger cubic
The staging integ alarm fired because the session knowledge-document inline-create check got a 405. #7179 moved tool operations in process and deleted the routes that only existed to serve them, but `createKnowledgeDocumentsContract` kept declaring `POST /api/knowledge/[id]/documents` — a path whose surviving GET/PATCH make Next.js answer POST with 405 rather than an honest 404. Nothing in the repo called it: the KB UI creates documents through the presigned upload flow, and the capability itself is unaffected because `knowledge_create_document` reaches the same use case in process. Audited all 1125 contracts for the same drift. It was the only one whose path resolves to a live route missing the declared method; 259 others declare paths of routes that were deleted outright, which 404 honestly and are left alone. - Drop the create-documents route contract for plain `params`/`body` schemas plus a named response schema, so nothing declares an endpoint we do not serve. The schemas stay in the contracts tree next to the siblings they share (`documentDataSchema` is used by the v2 contracts, and `createKnowledgeDocumentsBodySchema` already backed the in-process operation). - Delete four contracts with no consumer at all — both TTS contracts, docusign, and mistral. Their handlers own better schemas: TTS dispatches by `toolId` with eight per-provider schemas instead of one passthrough superset, and mistral bounds `pages` by the OCR request policy. crowdstrike and windchill look similar but are load-bearing (schema and derived types are imported by live code), so they stay. - Add `check:api-contract-routes`, picked up automatically by `run-audits`. `check:route-verbs` scans routes to contracts, so a contract whose route method was deleted is invisible to it — verified it passes clean against the exact regression this catches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tions Greptile flagged the audit's brace counter as blind to braces inside strings, template literals, regexes and comments. It was, but the bigger problem was that a text scan can only see contracts whose `method`/`path` are inline literals — the 70-plus built through `definePostSelector(path, …)` and friends were never checked at all. Comparing raw `defineRouteContract(` occurrences against parsed ones showed the scanner silently skipping declarations. Read the contracts by importing each contract module and inspecting its exported objects instead, the way `check-route-verbs.ts` already resolves the contract behind a route. Contract modules are pure Zod so importing them is safe; route files stay a static scan because importing one drags in `@sim/db`, auth and `next/server`. Barrels re-export the same object, so entries are keyed by identity. Coverage goes from 1125 contracts to 1283. That immediately surfaced two more instances of exactly what this PR retires. `/api/tools/confluence/page` kept its `PUT` and `DELETE` contracts after #7179 reduced the route to the selector `POST`, so both declared verbs the live route answers with 405. Neither is fetched — `lib/internal/confluence/execute-tool.ts` is the only consumer — so they become plain schemas like the knowledge one, and `executeOperation` now delegates to a schema form rather than growing a second pattern beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(api): retire route contracts that no route serves The staging integ alarm fired because the session knowledge-document inline-create check got a 405. #7179 moved tool operations in process and deleted the routes that only existed to serve them, but `createKnowledgeDocumentsContract` kept declaring `POST /api/knowledge/[id]/documents` — a path whose surviving GET/PATCH make Next.js answer POST with 405 rather than an honest 404. Nothing in the repo called it: the KB UI creates documents through the presigned upload flow, and the capability itself is unaffected because `knowledge_create_document` reaches the same use case in process. Audited all 1125 contracts for the same drift. It was the only one whose path resolves to a live route missing the declared method; 259 others declare paths of routes that were deleted outright, which 404 honestly and are left alone. - Drop the create-documents route contract for plain `params`/`body` schemas plus a named response schema, so nothing declares an endpoint we do not serve. The schemas stay in the contracts tree next to the siblings they share (`documentDataSchema` is used by the v2 contracts, and `createKnowledgeDocumentsBodySchema` already backed the in-process operation). - Delete four contracts with no consumer at all — both TTS contracts, docusign, and mistral. Their handlers own better schemas: TTS dispatches by `toolId` with eight per-provider schemas instead of one passthrough superset, and mistral bounds `pages` by the OCR request policy. crowdstrike and windchill look similar but are load-bearing (schema and derived types are imported by live code), so they stay. - Add `check:api-contract-routes`, picked up automatically by `run-audits`. `check:route-verbs` scans routes to contracts, so a contract whose route method was deleted is invisible to it — verified it passes clean against the exact regression this catches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(api): read contracts by import, and retire two more stale declarations Greptile flagged the audit's brace counter as blind to braces inside strings, template literals, regexes and comments. It was, but the bigger problem was that a text scan can only see contracts whose `method`/`path` are inline literals — the 70-plus built through `definePostSelector(path, …)` and friends were never checked at all. Comparing raw `defineRouteContract(` occurrences against parsed ones showed the scanner silently skipping declarations. Read the contracts by importing each contract module and inspecting its exported objects instead, the way `check-route-verbs.ts` already resolves the contract behind a route. Contract modules are pure Zod so importing them is safe; route files stay a static scan because importing one drags in `@sim/db`, auth and `next/server`. Barrels re-export the same object, so entries are keyed by identity. Coverage goes from 1125 contracts to 1283. That immediately surfaced two more instances of exactly what this PR retires. `/api/tools/confluence/page` kept its `PUT` and `DELETE` contracts after #7179 reduced the route to the selector `POST`, so both declared verbs the live route answers with 405. Neither is fetched — `lib/internal/confluence/execute-tool.ts` is the only consumer — so they become plain schemas like the knowledge one, and `executeOperation` now delegates to a schema form rather than growing a second pattern beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
Type of Change
Testing
Checklist