v0.8.16: aws lambda, dynamics 365, actorless run fixes, usage chart - #7224
Merged
Conversation
* feat(lambda): add AWS Lambda integration with 50 operations
Adds an AWS Lambda block covering every major resource family: invocation,
function CRUD, versions and aliases, resource-based permissions, event source
mappings, reserved and provisioned concurrency, function URLs, asynchronous
invocation configs, layers, tags, account settings, recursion detection, and
runtime management.
Tools run in-process through the shared internal tool-operation boundary using
@aws-sdk/client-lambda. Request and response shapes are contract-bound, with
shared response projections for the FunctionConfiguration, alias, event source
mapping, function URL, provisioned concurrency, and layer types.
Deployment packages are sourced from Amazon S3 or a container image URI; inline
.zip upload is not supported.
* chore(lambda): regenerate docs manifest for the new integration page
* fix(lambda): let update operations clear collection-valued settings
An empty list field coerced to undefined, so the AWS update command omitted
the field and the previous value survived — there was no way to remove a
function's layers, detach it from a VPC, or drop event source filters,
response types, and source access configurations.
A blank field still means "leave unchanged", since treating it as "clear"
would wipe the setting on every update that left it empty. Clearing is now
an explicit empty-array literal, documented in each field's description and
placeholder.
Also stops toSourceAccessConfigurations from folding an empty list back into
an omitted field.
* fix(lambda): stop the function timeout hijacking the tool request deadline
The `timeout` param name is reserved: the shared tool executor reads
`params.timeout` as its own operation deadline in milliseconds. A Lambda
function timeout of 30 seconds therefore aborted the call after 30ms, so
Create Function and Update Function Configuration failed whenever a timeout
was set. Renamed to `functionTimeout`.
Also tightens the boundary against documented AWS constraints and drops
avoidable provider round-trips:
- optional params supplied as null or an empty string are omitted rather than
forwarded, so an empty qualifier no longer reaches AWS as `Qualifier: ''`
- documented limits on functionName, qualifier, clientContext, statementId,
action, and alias routing weights
- source access configuration types are a closed enum, excluding VIRTUAL_HOST
on update where AWS rejects it
- cross-field checks: exactly one code source matched to packageType, runtime
and handler required for a .zip package, an event source or Kafka bootstrap
servers required, AT_TIMESTAMP paired with its timestamp, and masterRegion
only alongside functionVersion ALL
- architectures takes exactly one value, so it is no longer clearable
- the block declares AuthMode.ApiKey, correcting a catalog entry that
advertised the integration as needing no authentication
Removes the unreachable non-ok branch from every transformResponse: the shared
executor throws on any non-ok response before transformResponse runs.
Raises the internal tool-operation registry test budget, whose cost scales with
the number of registered tools.
* fix(lambda): never send a half-configured VPC attachment
Clearing only one of the two VPC lists produced `VpcConfig: { SubnetIds: [] }`
with no security groups, because an empty array is truthy and the wrapper
included each list independently. That is not a detach — it is an invalid
partial attachment.
A Lambda VPC attachment is a unit, so the contract now requires the two lists
to be supplied together on Create Function and Update Function Configuration,
naming the missing side. The wrapper defaults the other list to empty as well,
so it cannot emit a one-sided config even if called directly.
* improvement(tools): retire direct execution * fix(tools): update operation model input test * fix(browser-use): validate operation payloads * fix(tools): address provider operation review * fix(tools): harden provider operation contracts * fix(tools): close operation lifecycle gaps * fix(tools): update supabase buckets atomically
The free plan's 1,000 credits are a one-time grant, not a monthly allowance. Free accounts have no subscription, so usage resolves against the open default billing window (epoch to year 9999) and is never reset: cycle-close skips free plans outright, and weekly refresh is paid-only. - Pricing cards, in-app upgrade table, and pricing JSON-LD now read "Included credits" with per-plan cadence: 1,000 one-time for Free, 6,000/month and 25,000/month for Pro and Max - Weekly refresh values carry /week so both rows are self-describing - Comparison free-tier fact no longer claims a monthly grant - Library articles say one-time credits for the free plan - Docs FAQ paid-plan credits corrected to 6,000 and 25,000 - dify-alternatives also described a 50/200-credit daily refresh, which was replaced by the weekly refresh in #7113
…l-down (#7219) * improvement(usage): add a period-labelled chart to the workspace drill-down The Workspaces drill-down showed two ranked lists and no chart, and stated its window nowhere — the period picker lives on the list behind it, so the carried-over window was invisible once you were inside. - Draw the summary's headline, delta, and trend chart at the top of the drill-down, narrowed to that workspace - Label its section with the selected period, which is now the only place the drill-down states its window - Carry workspaceId through the summary contract, route, use case, query key, and hook so the chart reads one workspace - Move the workspace narrowing onto buildUsageAnalyticsScope, so the chart, the headline, and both lists derive it from one definition instead of the breakdown query owning a second copy The comparison window takes the same narrowing, or the delta would measure one workspace against the whole organization. No allowance figure is shown, unlike the Overview: the limit is pooled across the organization and would read as that workspace's own cap. * fix(usage): stop a retained summary from crossing workspace scopes The summary key now carries a workspaceId, and keepPreviousData retains across any key change — so moving between two drill-downs drew one workspace's headline, delta, and chart under the other's name until the fetch landed. - Narrow placeholderData to a period change only, matching the breakdown's existing scoped predicate, and share the one key-identity helper - Order the summary key so window is the trailing segment, making the scope a plain prefix as it already is on the breakdown - Give UsageSummary the isPlaceholderData signal UsageConsumers already takes, so retained figures dim instead of reading as fresh ones
…rations (#7194) A Redis command that never gets a reply fails identically whether the connection was still being established, was reconnecting with the command parked in the offline queue, or was a socket that had silently died. ioredis reports all three the same way — `Error: Command timed out` with only its own timer frames in the stack, no app frame naming the call, and no lifecycle event saying which happened. Nothing recorded anywhere distinguishes them, so the cause can only be inferred from timing. Adds `describeRedisConnection()`: client status, connection and ready ages, offline-queue depth, lifecycle counters, and whether the configured host is an IP or a DNS name. `status` alone usually decides it; queue depth confirms, since a parked command was waiting on connection setup while one written to a `ready` socket that never answered means the socket died unreported. Host kind rules DNS resolution in or out, which no server-side telemetry can see. Attaches it to the usage-reservation slot operations — the first Redis calls a queued workflow makes, so an unusable connection surfaces there first. Connect and ready now log elapsed-since-construction, making the wait before a connection becomes usable directly measurable; today it is spent inside a command's deadline, where it reads as a command timeout rather than as connection latency. Purely additive. Redis call arguments are unchanged, the wrapper rethrows the original error object, and `describeRedisConnection` never throws — it runs inside catch blocks where a throw would replace the real failure. All three have tests that fail if the behavior is removed. Only non-sensitive facts are derived from REDIS_URL, which carries the AUTH token and is never logged.
* fix(enterprise): verify invited owner on acceptance * test(enterprise): pin owner email update scope
* fix(logs): stop requiring a human subject on actorless runs Scheduled, public-API, and subject-less webhook runs carry no user on their principal. Several use cases resolved one with requirePrincipalSubjectUserId where the user was only attribution, so those runs failed with an opaque 500. Authorization for an actorless caller comes from the workflow running a deployment, never from a userId; each site now treats the user as what it actually is. * fix(mcp): keep unattended runs connecting as their execution actor Restores the identity MCP used before the Logs and MCP tools moved in-process: the executor minted an internal token from ExecutionContext.userId and the MCP route ran as that user, so a schedule, webhook, or public-API run has always reached MCP as the execution actor. Names it for what it is rather than implying it is the workflow's author, and documents that a workspace-level MCP identity is the real fix. Also types the missing-execution-context error so it is answered as unauthenticated rather than falling through to a generic 500.
Co-authored-by: Vikhyath Mondreti <vikhyath@simstudio.ai>
* feat(integrations): add Dynamics 365 CRM * fix(integrations): harden Dynamics environment requests * fix(integrations): align Dynamics paging and lifecycle contracts * fix(integrations): validate Dynamics 365 contracts * fix(integrations): harden Dynamics OAuth binding * improvement(integrations): simplify Dynamics OAuth UI * fix(docs): sync Dynamics integration manifest * test(integrations): isolate Dynamics OAuth mutation --------- Co-authored-by: Bill Leoutsakos <billleoutsakos@Bills-MacBook-Pro.local> Co-authored-by: Waleed Latif <walif6@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
* feat(secrets): reveal visible values to members * fix(secrets): harden visible value reveal * fix(secrets): reveal legacy values safely
…al (#7228) * fix(execution): treat an undetermined lease as a fallback, not a denial The distributed owner lease is a cross-process fairness check, not a correctness lock. A round trip that did not answer before its deadline was reported as a hard failure and rejected the execution, even though the per-process pool and the per-owner active/queued limits still bound the work. - Fall back to the local limits when the lease is undetermined. Only `limit_exceeded` denies an execution, since it is an actual answer. - Rename that outcome from `unavailable` to `undetermined` so the absence of an answer is not read as a negative one, and log it at warn. - Make the round-trip deadline configurable and raise its default. This deadline and the client's `commandTimeout` are both plain timers, so a value near normal event-loop latency misreads a scheduling pause as an unreachable dependency. - Skip the release round trip when no lease was ever registered. * fix(execution): reclaim a lease Redis registers after the local deadline Addresses review findings on the fallback path. - Always release the lease. The deadline abandons the local wait but cannot cancel the script, so a late completion still registers the lease id; leaving it unreleased kept it counted against the owner for the whole TTL and denied later executions that did have capacity. The id is unique per execution, so removing one that was never registered is a no-op. - Treat a non-positive configured deadline as unconfigured. A timer of zero or less fires immediately, which would leave every acquisition undetermined and silently drop cross-replica enforcement. - Cover both with tests: a lease that completes after the deadline is still released, and a non-positive deadline still lets a real answer land. * test(execution): stop the lease deadline override leaking between tests - Add `IVM_LEASE_REDIS_DEADLINE_MS` to the harness env reset. It was absent, so a test that overrode it left the value in the module-scoped mock env for every later test in the file, quietly changing their fallback timing. - Drop the duplicate over-limit test and fold its extra assertion into the existing one; the two had identical setup and covered the same path.
* fix(guardrails): route PII validation through app runtime * fix(guardrails): bound PII validation results
* fix(executor): preserve actors for actorless tool calls * fix(auth): bind legacy execution actors to principals
Contributor
|
Too many files changed for review (567 files, 100 file limit). |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 567 files
Confidence score: 2/5
apps/sim/lib/internal/datadog/operations/update-slo.tscan send Datadog credentials to an attacker-controlledapi.<site>whensiteis arbitrary, creating a serious credential disclosure risk — validate the value against the runtimeDatadogSiteallowlist before building the URL.apps/sim/lib/internal/google-drive/operations.tsmay include an existing destination parent in bothaddParentsandremoveParents, causing Drive to reject the update or remove the intended parent — exclude the destination from removals.apps/sim/lib/internal/bitbucket/operations/get-file.tscan request an invalid range for zero-byte text files, whileapps/sim/app/workspace/[workspaceId]/settings/components/secrets/components/secret-value-field/secret-value-field.tsxcan place the caret incorrectly when editing masked secrets — short-circuit empty files and preserve the real secret length when positioning the caret.- Runtime validation remains fragile in
apps/sim/lib/internal/cbinsights/operations/rag.ts,apps/sim/lib/internal/managed-agent/operations/respond-tool-confirmation.ts, andapps/sim/lib/internal/cbinsights/operations/get-exit-probability-history.ts: boundary or non-string inputs can bypass contracts or throw instead of returning validation responses — enforce the documented types and limits before trimming or invoking downstream operations.
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/bitbucket/operations/get-file.ts">
<violation number="1" location="apps/sim/lib/internal/bitbucket/operations/get-file.ts:28">
P2: When metadata reports a zero-byte text file, this requests `Range: bytes=0-...` and can receive 416 instead of returning the valid empty file. Short-circuit zero-size files with `content: ''` before the raw request.</violation>
</file>
<file name="apps/sim/lib/internal/cbinsights/operations/rag.ts">
<violation number="1" location="apps/sim/lib/internal/cbinsights/operations/rag.ts:10">
P2: When `message` is exactly 10,000 characters, this guard allows it despite the tool contract requiring fewer than 10,000 characters. Reject the boundary value before calling CB Insights.</violation>
</file>
<file name="apps/sim/lib/internal/google-drive/operations.ts">
<violation number="1" location="apps/sim/lib/internal/google-drive/operations.ts:324">
P2: When the destination is already a current parent, this sends it in both `addParents` and `removeParents`, so Drive can reject the update or remove the destination parent. Exclude the destination from `removeParents` and omit the parameter when no parents remain.</violation>
</file>
<file name="apps/sim/lib/internal/managed-agent/operations/respond-tool-confirmation.ts">
<violation number="1" location="apps/sim/lib/internal/managed-agent/operations/respond-tool-confirmation.ts:39">
P2: When a malformed request supplies a non-string `denyMessage`, `.trim()` throws before the `try` block, so the operation escapes without a `ToolResponse`. Accept only strings (or coerce the value) before trimming.</violation>
</file>
<file name="apps/sim/lib/internal/cbinsights/operations/get-exit-probability-history.ts">
<violation number="1" location="apps/sim/lib/internal/cbinsights/operations/get-exit-probability-history.ts:20">
P2: When `startDate` or `endDate` arrives with a non-string runtime value, optional chaining does not protect `.trim()`, so this operation throws a `TypeError` instead of returning the existing descriptive validation error. Use `parseOptionalStringParam` for both date parameters, as the sibling history operation does.</violation>
</file>
<file name="apps/sim/app/workspace/[workspaceId]/settings/components/secrets/components/secret-value-field/secret-value-field.tsx">
<violation number="1" location="apps/sim/app/workspace/[workspaceId]/settings/components/secrets/components/secret-value-field/secret-value-field.tsx:59">
P2: When an editable secret is longer than ten characters, clicking to edit it computes the caret against the ten-character mask before focus replaces it with the real value, so typing inserts at the wrong offset. Keep the real value in editable inputs and apply visual masking, or remap the selection when revealing it.</violation>
</file>
<file name="apps/sim/lib/internal/datadog/operations/update-slo.ts">
<violation number="1" location="apps/sim/lib/internal/datadog/operations/update-slo.ts:14">
P1: When an operation input contains an arbitrary `site`, this URL builder sends the Datadog credentials to `api.<site>`. Validate `site` against the fixed `DatadogSite` allowlist at the runtime boundary before constructing the URL, and reject unknown sites.</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.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
Collaborator
|
Triaged the 33 open Cubic findings on this release against the workspace storage lock change from #7187. No overlap. None of the findings touch any of that PR's 11 changed files, and none touch row locking, transaction ordering, storage accounting, or workspace payer semantics. They group as: AWS Lambda contract bounds (17), Dataverse/Dynamics 365 OAuth and credential UI (5), CB Insights input guards (2), overflow-tooltip UI (2), and one each in Datadog, Google Drive, Bitbucket, managed-agent, isolated-vm, the secret value field, and a skill doc. The nearest neighbour is the isolated-vm finding, which concerns a Redis execution lease rather than a database row lock. Verified the change is intact as merged here: every #7187 was reviewed at 5/5 with zero open threads, so no follow-up code change is needed for that scope. |
Collaborator
|
Reviewed the 33 Cubic findings against the integration-defects change from #7195 (Confluence Zero of the 33 threads touch any of that PR's 70 changed files. The one with a real connection is the I also re-verified that #7207's retirement carried the #7195 GitHub fix forward intact rather than dropping it in the rewrite. The rest of the #7195 surface is byte-identical on staging (46 Confluence tools, 5 Drive tools plus the block, Vanta, the subblock migration, No further code change needed for that scope. |
… checklist (#7236) The transport checklist listed the property inside a bare enumeration -- "declares `request.internal`, or `directExecution`, or has an HTTP fallback" -- which stacks two disjunctions and drops the "retired" framing every other statement of this rule carries, including line 71 of the same file and the add-tools, add-trigger, and add-block skills. A reader scanning the checklist could take it for a property that still exists and is merely discouraged. Wording only; the rule itself is unchanged and is enforced by check:tool-request-boundary.
…quisition (#7237) * fix(execution): release the lease when abort races an undetermined acquisition The cancellation return for an aborted execution ran before `releaseLease` was declared, so it exited without releasing. An undetermined acquisition can still be registered by Redis after the deadline abandons the local wait, and that member then counted against the owner for the whole TTL, denying later executions that had capacity. Declare the release before the early returns so every exit path that can leave a registration behind reaches it, and call it on the cancellation path. The over-limit return still skips it: the script answers that before its ZADD, so nothing was ever registered. * fix(execution): keep the over-limit abort path release-free The abort guard fires for any non-acquired result, so an over-limit acquisition that was already aborted also released — contradicting the rule stated two lines below, where the same result returns without releasing because the script answers before its ZADD. Restrict the release to undetermined results and assert the no-release behavior in the existing over-limit abort test.
* fix(dataverse): harden OAuth connection preflight * fix(dataverse): preserve legacy reconnect behavior
6 tasks
Collaborator
|
Went through the review threads on this release PR and handled the 17 that land on the AWS Lambda integration from #7216. Fix is up as #7241 against 16 were real and are fixed. Two were genuine holes rather than missing bounds:
The rest are documented AWS bounds — alias names (1–128 plus the pattern that rejects all-digit names), descriptions (256), layer names (140 plus the name-or-ARN pattern), optional One I did not change: Two details worth flagging for anyone reading the diff:
I also scoped a couple of requests down where the reference doesn't back them: no per-key maximum on The remaining open threads on this PR belong to the other changes in the release (Dataverse, Datadog, Bitbucket, CB Insights, Google Drive, managed agents, isolated-vm, secrets, sidebar) and are for those authors. |
* feat(credential-groups): add seventeen OAuth providers Adds Airtable, Asana, Attio, Box, Cal.com, ClickUp, DocuSign, Dropbox, HubSpot, Linear, LinkedIn, monday.com, Notion, Pipedrive, Salesforce, WordPress.com and Zoom to Credential Groups. * fix(credential-groups): hydrate the list cache with the whole response The settings prefetch seeded the shared list key with only the groups array, so every consumer read an empty list for as long as the hydrated value stayed fresh. Also resets the account-type filter when a group opens or closes, alongside the tab it already reset.
Co-authored-by: Sim Pi Agent <pi@sim.ai>
* fix(lambda): close the remaining contract validation gaps Follow-up to #7216, from review on the release PR. Every bound added here is one the AWS Lambda API reference documents; findings that asked for undocumented limits were left alone. Code source selection had two holes. `hasS3` required both S3 fields, so a partial pair alongside `imageUri` read as "image only" and the stray S3 field still went to AWS; and an `imageUri` with no `packageType` was accepted even though the package type then defaults to Zip. Both now fail at the boundary, naming the field to change, and the zip-only `s3ObjectVersion` and `sourceKmsKeyArn` count as S3 fields for the exclusivity check. The VPC guard only checked that both lists were supplied, so supplying both with one empty passed and produced a partial update. Both must now be empty (detach) or both populated (attach). Documented bounds added: - alias names: 1-128 and the documented pattern, which excludes all-digit names - descriptions: 256 characters - layer names: 140 characters and the name-or-ARN pattern - optional `functionName` on the event source mapping operations: 1-256 - `RemovePermission` statement IDs: 1-100 and its own pattern, which allows a dot where `AddPermission` does not Also rejects values that are structurally meaningless rather than merely out of range: empty tag keys, empty Kafka bootstrap servers, more than one weighted routing entry, and an event source mapping that supplies both an event source ARN and self-managed Kafka bootstrap servers. * fix(lambda): match twelve digits in the layer ARN pattern The account-ID segment was written as `\d{12}` inside a template literal, so the emitted regex carried a literal `d{12}` and rejected every real layer ARN. The existing test only covered an over-long name, which is why it passed. Escapes the backslash and adds the coverage that would have caught it: a real layer ARN and a bare layer name are both accepted, and an ARN whose account segment is not twelve digits is rejected. * fix(lambda): reject empty code-source fields instead of ignoring them The mutual-exclusivity check used truthiness, so `imageUri` alongside `s3Bucket: ''` read as image-only while the create operation still forwarded the defined empty field to AWS. An empty string is meaningless for every code-source field, so each is now `.min(1)` at the contract rather than special-cased in the refinement. * fix(lambda): reject empty optional strings across the Lambda contracts An empty `eventSourceArn` alongside bootstrap servers slipped past the mutual-exclusivity check for the same reason the code-source fields did: the guard tests truthiness, so a defined-but-empty value reads as absent while the operation still forwards it. Rather than patch each field as it surfaces, every optional string field now rejects an empty value. The tool layer already drops `''` before it reaches a contract, so an empty value can only arrive from a malformed direct call, and forwarding it to AWS is never right. `description` is exempt: AWS documents it as "Minimum length of 0", so an empty value legitimately clears it. Both behaviours are covered by tests. * fix(lambda): stop rejecting values AWS documents as valid A comprehensive validation pass against the API reference found the previous commit's blanket "no empty optional strings" rule was wrong. Several Lambda parameters document an empty string as meaningful, and their patterns say so: KMSKeyArn, SourceKMSKeyArn, and DeadLetterConfig.TargetArn all carry `(arn:...)|()`, whose trailing alternative matches the empty string, and the on-success/on-failure destinations document `Minimum length of 0` with a pattern beginning `$|`. For each, empty is how the setting is cleared. The rule is now opt-in rather than opt-out: only the five fields feeding a truthiness-based cross-field check reject an empty value. That removes 47 constraints and leaves the ones that were actually reported. Also from the same pass: - Supplying an image URI no longer demands an explicit `packageType`. That subBlock is advanced with no default, so requiring it produced a 400 naming a control the user cannot see; the operation derives Image from the code source instead, and only an explicit Zip alongside an image is rejected. - `fileSystemConfigs` was the one projection without a null guard, so an omitted field vanished from the block output rather than reading null. - An absent function URL now maps to null instead of an empty string a workflow could build a request against. - GetFunction reports `tagsError`, so a partial tag-read failure is distinguishable from a function with no tags, and marks `configuration` nullable to match what the operation returns. - Event source mappings report `selfManagedKafkaBootstrapServers`, which could be set but never read back. - TagResource rejects an empty tag map instead of reporting "0 tags applied". * test(lambda): prove every projection matches its contract schema Reading code confirmed the projections and schemas agree, but nothing ran them against each other. This runs all eight shared mappers against their schemas in both directions: every declared key is emitted and non-undefined, no undeclared key is emitted, and the result parses — for an empty AWS response, which is the common case, and for a fully-populated one. Verified the suite fails when either defect class is reintroduced: a mapper that stops emitting a declared key, and a projection that leaks `undefined` where the schema declares a value.
…s to constrain (#7244) * fix(integrations): validate runtime values the type system only claims to constrain Three defects surfaced by review of the v0.8.16 release PR (#7224), each one a declared type standing in for a check that never runs. datadog: `DatadogSite` is a compile-time union, erased at runtime, and `site` is interpolated straight into the request host while every Datadog request carries DD-API-KEY and DD-APPLICATION-KEY. An unvalidated value therefore chose where the workspace's Datadog credentials were sent: `evil.com` addresses api.evil.com, and `datadoghq.com@evil.com` addresses evil.com with the expected host as userinfo. The site list is now a runtime array with the type derived from it, and both host builders -- `datadogApiUrl` (31 call sites) and the logs intake in send_logs -- resolve through one validator. Not reachable from the editor today, since the block renders a dropdown and the param is user-only; the value still survives in stored workflow state, which imports and programmatic edits write directly. cbinsights: `params.x?.trim()` guards undefined, not the type, so a block-to-block reference resolving to a number threw a bare TypeError naming no parameter. The sibling history operation already used `parseOptionalStringParam`; the remaining 19 sites now do too. Behaviour is otherwise unchanged -- `compactBody` already dropped '' and undefined alike, and every non-compactBody use tests falsiness. cbinsights rag: the guard admitted a 10,000-character message while its own error and the tool's param description both say "under 10,000". managed-agent: `denyMessage` was trimmed above the try block, so a non-string threw past every `success: false` path the operation otherwise returns. It is now coerced the same way `decision` is a few lines above. Two further findings on that PR were checked and left alone: the Drive addParents/removeParents overlap matches Google's own documented move sample in all four languages, and Bitbucket's Range behaviour on a zero-byte file is undocumented, so neither is a verified defect. * fix(datadog): route every host builder through the allowlist, coerce scalars safely Review round 1 on #7244 found the first pass incomplete, and both findings reproduce. The site allowlist only covered the shared `datadogApiUrl` and the logs intake. Ten tools build the host inline -- `const site = params.site || 'datadoghq.com'` in cancel_downtime, create_downtime, create_event, create_monitor, get_monitor, list_downtimes, list_monitors, query_logs, query_timeseries and submit_metrics -- so they never reached the validator while still attaching DD-API-KEY and, where the endpoint needs it, DD-APPLICATION-KEY. All ten now resolve through it. The new test sweeps the tool registry rather than naming tools, so a future tool that reintroduces an inline builder fails instead of shipping an unguarded request. `(value ?? '').toString()` was itself unsafe: an object whose `toString` is not a function, and one with a null prototype, both throw TypeError, and `String(value)` throws on the same two. That read sits above the try block, so it escaped the structured `success: false` result this operation promises. `normalizeScalarText` converts only the scalar kinds `String()` cannot fail on and returns '' otherwise, matching how `normalizeStringList` already treats a value of the wrong type. The identical hazard on `decision` two lines above is fixed with it as well.
Co-authored-by: Sim Pi Agent <pi@sim.ai>
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
4 participants
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.
Uh oh!
There was an error while loading. Please reload this page.