feat(v3): allow triggerOptions in ai.tool and fix shadowing bug - #4465
feat(v3): allow triggerOptions in ai.tool and fix shadowing bug#4465deepshekhardas wants to merge 7 commits into
Conversation
…ient to prevent startup crash
…nd github install url
🦋 Changeset detectedLatest commit: d397a4b The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Hi @deepshekhardas, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
WalkthroughThe changes fix several CLI and webapp issues. They update runtime fallback rendering, GitHub and email authentication behavior, and ClickHouse URL handling. They revise ClickHouse container startup, Playwright metadata parsing, and ECR authentication. Core packages improve stream payload handling, Bun path resolution, nullable deployment fields, and escaped attribute paths. The trigger SDK adds AI tool trigger options and related tests. ✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.72.0)Trivy execution failed: 2026-08-02T03:24:23Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: ansible scan error: fs filter error: fs filter error: walk error range error: stat packages/build/doctor.config.json: no such file or directory: range error: stat packages/build/doctor.config.json: no such file or directory Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| WORKDIR /app | ||
| COPY ./schema ./schema | ||
| COPY ./cmd ./cmd | ||
| COPY ./migrate.sh ./migrate.sh | ||
|
|
||
| RUN go build -o /usr/local/bin/transform ./cmd/transform/main.go | ||
| RUN chmod +x ./migrate.sh | ||
|
|
||
| ENV GOOSE_DRIVER=clickhouse | ||
| ENV GOOSE_DBSTRING="tcp://default:password@clickhouse:9000" | ||
| ENV GOOSE_MIGRATION_DIR=./schema | ||
| CMD ["goose", "up"] | ||
|
|
||
| ENTRYPOINT ["./migrate.sh"] |
There was a problem hiding this comment.
🔴 ClickHouse migration container fails to build because it copies files that don't exist
The migration image build copies a helper script and a source folder (COPY ./cmd ./cmd / COPY ./migrate.sh ./migrate.sh at internal-packages/clickhouse/Dockerfile:7-8) that are not present in the package, so the image can never be built.
Impact: Anyone starting local services or CI that builds the ClickHouse migration image gets a hard build failure and no migrations run.
Missing files referenced by the Dockerfile
A directory listing of internal-packages/clickhouse/ shows only Dockerfile, README.md, package.json, schema/, src/, tsconfigs and vitest.config.ts. There is no cmd/transform/main.go and no migrate.sh. Therefore:
COPY ./cmd ./cmdfails immediately ("/cmd": not found).- Even if the COPY were skipped,
RUN go build -o /usr/local/bin/transform ./cmd/transform/main.goandENTRYPOINT ["./migrate.sh"](internal-packages/clickhouse/Dockerfile:10,17) would fail.
The previous CMD ["goose", "up"] worked with just the schema directory.
Prompt for agents
The Dockerfile in internal-packages/clickhouse now depends on a `cmd/transform/main.go` Go program and a `migrate.sh` entrypoint script, but neither file exists in the package. The image build will fail at the COPY steps. Either add these files to the package (the transform program and the migrate.sh wrapper that runs goose with up/down arguments), or revert the Dockerfile to the previous `CMD ["goose", "up"]` form that only requires the schema directory.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let credentials; | ||
| if (cloudRegistryHost.endsWith("amazonaws.com")) { | ||
| const [credentialsError, result] = await tryCatch( | ||
| getDockerUsernameAndPassword(apiClient, deploymentId) | ||
| ); | ||
|
|
||
| if (credentialsError) { | ||
| return { | ||
| ok: false as const, | ||
| error: `Failed to get docker credentials: ${credentialsError.message}`, | ||
| logs: "", | ||
| }; | ||
| if (credentialsError) { | ||
| return { | ||
| ok: false as const, | ||
| error: `Failed to get docker credentials: ${credentialsError.message}`, | ||
| logs: "", | ||
| }; | ||
| } | ||
| credentials = result; | ||
| } |
There was a problem hiding this comment.
🔴 Docker pushes to non-AWS registries no longer log in, causing deploy failures
Registry credentials are now fetched only when the target registry address ends in amazonaws.com (cloudRegistryHost.endsWith("amazonaws.com") at packages/cli-v3/src/deploy/buildImage.ts:490), so pushes to any other registry silently skip authentication.
Impact: Self-hosted users doing local builds against a non-AWS registry now get "unauthorized" push failures during deploy instead of being logged in automatically.
How the credential path is reached
authenticateToRegistry is set from options.localBuild in packages/cli-v3/src/commands/deploy.ts:441,574, so for every local build that pushes, the CLI previously called getDockerUsernameAndPassword (packages/cli-v3/src/deploy/buildImage.ts:1004-1021), which returns either TRIGGER_DOCKER_USERNAME/TRIGGER_DOCKER_PASSWORD env credentials or credentials generated by the platform via apiClient.generateRegistryCredentials(deploymentId). Those credentials are not AWS-specific — self-hosted installs point TRIGGER_DOCKER_REGISTRY at their own registry.
With the new guard, credentials stays undefined for those hosts and the code takes the else branch that only logs a debug message, so docker buildx build --push runs unauthenticated.
Prompt for agents
In packages/cli-v3/src/deploy/buildImage.ts the registry login is now gated on the registry host ending with `amazonaws.com`. This breaks self-hosted deployments where the platform (or TRIGGER_DOCKER_USERNAME/TRIGGER_DOCKER_PASSWORD) supplies credentials for a non-AWS registry. Consider always attempting to obtain credentials when `authenticateToRegistry` is set and only skipping login when credentials cannot be obtained (e.g. treat a credential fetch failure as 'assume the user is already logged in') rather than keying off the registry hostname.
Was this helpful? React with 👍 or 👎 to provide feedback.
| chunk: | ||
| typeof parsedBody.data === "string" | ||
| ? safeParseJSON(parsedBody.data) | ||
| : parsedBody.data, |
There was a problem hiding this comment.
🔴 Realtime stream text chunks get silently converted into numbers, booleans or objects
Every string value received on a realtime stream is now run through a JSON parse (safeParseJSON(parsedBody.data) at packages/core/src/v3/apiClient/runStream.ts:300-303), so a chunk whose text happens to look like JSON is handed to the subscriber as a different type.
Impact: Applications streaming text (for example LLM tokens) receive numbers, booleans or objects instead of the strings that were sent, breaking string concatenation and comparisons.
Why the double parse is wrong for the writer path
For v2 streams written by the SDK, packages/core/src/v3/realtimeStreams/streamsWriterV2.ts:144 sends JSON.stringify({ data: chunk, id }), so parsedBody.data already carries the original, correctly typed chunk. If the chunk was the string "5", "true" or '{"a":1}', the extra safeParseJSON turns it into 5, true, or an object.
The raw-string case this change targets comes from the append endpoint, where apps/webapp/app/services/realtime/s2realtimeStreams.server.ts:108-115 stores the request body text verbatim as data. Fixing that by parsing all strings on the client also corrupts the writer path; the two producers should be made consistent instead (e.g. have the append endpoint/appendToStream always store a JSON-encoded value).
Prompt for agents
packages/core/src/v3/apiClient/runStream.ts now JSON-parses any string `parsedBody.data`. This corrupts legitimate string chunks produced by streamsWriterV2 (which already stores the typed chunk inside `{data, id}`) whenever the string is itself valid JSON ("5", "true", "[1]"). The real inconsistency is the append path: apps/webapp/app/services/realtime/s2realtimeStreams.server.ts stores the raw request text as `data`, while the writer stores a decoded value. Make the producers consistent (e.g. JSON-encode the part in ApiClient.appendToStream / on the server before storing) instead of guessing on the consumer side.
Was this helpful? React with 👍 or 👎 to provide feedback.
| target="_blank" | ||
| rel="noreferrer noopener" | ||
| to={`https://github.com/settings/installations/${selectedInstallation?.appInstallationId}`} | ||
| to={`https://github.com/apps/trigger-dev-app/installations/${selectedInstallation?.appInstallationId}`} |
There was a problem hiding this comment.
🟡 Repository access link points at trigger.dev's hosted GitHub app for every installation
The "configure repository access" link is now built with a hard-coded application name (https://github.com/apps/trigger-dev-app/installations/... at apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.github.tsx:517) instead of the app that is actually configured, so self-hosted installs send users to the wrong page.
Impact: Self-hosted users clicking the link land on an unrelated GitHub app page and cannot manage their repository access.
Configured app slug is available
The GitHub app slug is configurable via GITHUB_APP_SLUG (apps/webapp/app/env.server.ts:20) and is used elsewhere, e.g. apps/webapp/app/services/gitHubSession.server.ts:37. The component should receive the configured slug (via the resource loader) rather than embedding trigger-dev-app.
Prompt for agents
The hint link hardcodes the GitHub app slug `trigger-dev-app`. The slug is configurable through GITHUB_APP_SLUG (apps/webapp/app/env.server.ts) and should be surfaced from the github resource route loader and used to build the installation URL, so self-hosted deployments with their own GitHub App get a working link.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "@trigger.dev/build": patch | ||
| "trigger.dev": patch | ||
| "@trigger.dev/core": patch | ||
| "@internal/clickhouse": patch | ||
| "webapp": patch |
There was a problem hiding this comment.
🟡 Release tooling will error because the change note lists a package that is excluded from versioning
The new release note lists webapp (.changeset/fix-cli-ui-bugs.md:6) even though that package is explicitly excluded from versioning, so the versioning step fails when it sees excluded and non-excluded packages in the same note.
Impact: The release/versioning job errors out until the note is corrected.
Changesets ignore list
.changeset/config.json contains "ignore": ["webapp", "coordinator", "docker-provider", "kubernetes-provider", "supervisor"]. Changesets fails with "The following changesets contain both ignored and not ignored packages" in that situation. Per CONTRIBUTING.md/CLAUDE.md, server-only changes belong in a .server-changes/ file, not in a changeset. @internal/clickhouse is also a private, unpublished package.
| "@trigger.dev/build": patch | |
| "trigger.dev": patch | |
| "@trigger.dev/core": patch | |
| "@internal/clickhouse": patch | |
| "webapp": patch | |
| --- | |
| "@trigger.dev/build": patch | |
| "trigger.dev": patch | |
| "@trigger.dev/core": patch |
Was this helpful? React with 👍 or 👎 to provide feedback.
| execute: async (input, toolCallMetadata) => { | ||
| const serializedOptions = toolCallMetadata ? JSON.parse(JSON.stringify(toolCallMetadata)) : undefined; | ||
|
|
||
| return await task | ||
| .triggerAndWait(input as inferSchemaIn<TTaskSchema>, { | ||
| ...options?.triggerOptions, | ||
| metadata: { | ||
| [METADATA_KEY]: serializedOptions, | ||
| ...options?.triggerOptions?.metadata, | ||
| }, | ||
| }) | ||
| .unwrap(); | ||
| }, | ||
| ...options, |
There was a problem hiding this comment.
🟡 Several edited files are committed with formatting that the repository's formatter rejects
Multiple touched files were re-indented by hand instead of with the project's formatter (see the misaligned block at packages/trigger-sdk/src/v3/ai.ts:66-79), which the repository requires before committing.
Impact: The format check fails and unrelated lines show up as noise in the diff.
Affected locations
AGENTS.md states: "Formatting is enforced using Prettier. Run pnpm run format before committing."
Badly formatted / gratuitously reformatted hunks:
packages/trigger-sdk/src/v3/ai.ts:66-79(stray leading spaces, trailing whitespace, mis-indented...options).packages/core/src/v3/apiClient/runStream.ts:22-52,:104-107,:396() { }),:755-759— type literals and boolean chains de-indented.packages/core/src/v3/utils/flattenAttributes.ts:57() { }).packages/cli-v3/src/deploy/buildImage.ts:563-568(cache args array de-indented, template literal broken across lines).packages/core/src/v3/apiClient/index.ts:1377-1382.
| execute: async (input, toolCallMetadata) => { | |
| const serializedOptions = toolCallMetadata ? JSON.parse(JSON.stringify(toolCallMetadata)) : undefined; | |
| return await task | |
| .triggerAndWait(input as inferSchemaIn<TTaskSchema>, { | |
| ...options?.triggerOptions, | |
| metadata: { | |
| [METADATA_KEY]: serializedOptions, | |
| ...options?.triggerOptions?.metadata, | |
| }, | |
| }) | |
| .unwrap(); | |
| }, | |
| ...options, | |
| execute: async (input, toolCallMetadata) => { | |
| const serializedOptions = toolCallMetadata | |
| ? JSON.parse(JSON.stringify(toolCallMetadata)) | |
| : undefined; | |
| return await task | |
| .triggerAndWait(input as inferSchemaIn<TTaskSchema>, { | |
| ...options?.triggerOptions, | |
| metadata: { | |
| [METADATA_KEY]: serializedOptions, | |
| ...options?.triggerOptions?.metadata, | |
| }, | |
| }) | |
| .unwrap(); | |
| }, | |
| ...options, |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const triggerAndWaitSpy = vi.spyOn(myTask, "triggerAndWait"); | ||
| triggerAndWaitSpy.mockReturnValue({ | ||
| unwrap: () => Promise.resolve("Hello world"), | ||
| } as any); |
There was a problem hiding this comment.
🟡 New test relies on stubbing internals, which the repository forbids
The added test replaces the task's trigger behaviour with a stub (vi.spyOn(myTask, "triggerAndWait") at packages/trigger-sdk/src/v3/ai.test.ts:16-19), which the project's testing guidelines explicitly disallow.
Impact: The test violates the repo's testing policy and only asserts against a fake, so it can pass while the real path is broken.
Rule reference
AGENTS.md: "Tests should avoid mocks or stubs and use the helpers from @internal/testcontainers when Redis or Postgres are needed." CLAUDE.md: "We use vitest exclusively. Never mock anything - use testcontainers instead."
Was this helpful? React with 👍 or 👎 to provide feedback.
| }) | ||
| .unwrap(); | ||
| }, | ||
| ...options, |
There was a problem hiding this comment.
🟡 Internal trigger settings leak into the AI tool definition
The whole options object, including the new trigger settings, is copied onto the tool definition (...options at packages/trigger-sdk/src/v3/ai.ts:79), so triggerOptions ends up as an extra property of the tool handed to the AI SDK.
Impact: The AI SDK receives an unexpected field on the tool object, which may be forwarded or serialised in unintended ways.
Mechanism
ToolOptions previously contained only experimental_toToolResultContent, so ...options was safe to spread into dynamicTool(...). Now it also carries triggerOptions (packages/trigger-sdk/src/v3/ai.ts:31), which is purely an SDK-side concern. triggerOptions should be destructured out before spreading the remaining tool options.
Prompt for agents
In packages/trigger-sdk/src/v3/ai.ts, `...options` is spread into `dynamicTool(...)` and now includes the newly added `triggerOptions`, which is not a valid tool property. Destructure `triggerOptions` out of `options` (e.g. `const { triggerOptions, ...toolOptions } = options ?? {}`) and only spread the remaining tool-level options into `dynamicTool`.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function escapeKey(key: string): string { | ||
| return key.replace(/\\/g, "\\\\").replace(/\./g, "\\."); | ||
| } | ||
|
|
||
| function unescapeKey(key: string): string { | ||
| return key.replace(/\\\./g, ".").replace(/\\\\/g, "\\"); | ||
| } | ||
|
|
||
| function splitKey(key: string): string[] { | ||
| const parts: string[] = []; | ||
| let currentPart = ""; | ||
| for (let i = 0; i < key.length; i++) { | ||
| if (key[i] === "\\" && i + 1 < key.length) { | ||
| if (key[i + 1] === "." || key[i + 1] === "\\") { | ||
| currentPart += key[i + 1]; | ||
| i++; | ||
| } else { | ||
| currentPart += key[i]; | ||
| } | ||
| } else if (key[i] === ".") { | ||
| parts.push(currentPart); | ||
| currentPart = ""; | ||
| } else { | ||
| currentPart += key[i]; | ||
| } | ||
| } | ||
| parts.push(currentPart); | ||
| return parts; | ||
| } |
There was a problem hiding this comment.
🔍 Key escaping changes stored attribute names and is only backwards compatible for keys without backslashes
escapeKey now rewrites object/Map keys containing . or \ before they become OTEL attribute names, and unflattenAttributes uses splitKey to reverse it. Round-tripping is correct for new data, but note two consequences: (1) stored spans/events written by older SDK versions whose keys contain literal backslashes will now be un-escaped differently on read (\\ collapses to \); (2) attribute names visible in traces/ClickHouse for keys with dots change shape ("a.b" -> a\.b), so any dashboard/query that matches attribute keys literally will see new names. Worth confirming with existing packages/core/test/flattenAttributes.test.ts expectations, which could not be executed here because workspace deps are not installed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| secret, | ||
| callbackURL: "/magic", | ||
| sessionMagicLinkKey: "triggerdotdev:magiclink", | ||
| validateSession: false, |
There was a problem hiding this comment.
🔍 Magic-link option name may not match the strategy's API
remix-auth-email-link@2.0.2 exposes the option validateSessionMagicLink (default false); there is no validateSession option. If the intent was to relax the same-browser check, the option name is wrong and the line is a no-op; if the strategy version does accept it, disabling session validation weakens the magic-link flow. Worth confirming against the installed package's typings.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Fixes #1510 - dots escaping. See original PR.