Repository navigation
fix(testing-sdk): only accept strings for sendCallbackSuccess - #6
Merged
Merged
Conversation
ParidelPooya
approved these changes
Sep 3, 2025
ParidelPooya
added a commit
that referenced
this pull request
Jul 5, 2026
…oc comment Addresses three review findings: #4 run_javascript return was unbounded — the value is JSON-stringified into the toolResult sent back to Bedrock. Now reject a serialized result over JS_RESULT_MAX_CHARS (20k) and ask the model to return a summary, so a large array can't inflate tokens/cost. #2 JS aggregates over a LIMIT-ed exploration query could be silently partial: the truncation note only fired when JS_ROW_CAP capped the rows, not when the source query itself carried LIMIT (rowCount then equals the LIMIT). run_query now records its source query; run_javascript adds a note when it contains a LIMIT, warning that a total/median/sum may be partial. Notes are now a list so both conditions can surface. #6 the recordTurn doc comment had slipped above withConversationContext (added during editing), leaving recordTurn undocumented — moved it back. 2 new tests (LIMIT note, oversized-result rejection); suite now 65. Typecheck + build clean.
ParidelPooya
added a commit
that referenced
this pull request
Aug 2, 2026
…erved ids Review items 2, 6, 11, 12. All four reproduced first. #2 -cdk never declared the SDK its own output imports. Every generated handler emits `from "@aws/durable-execution-sdk-js"`, but package.json listed only visual-workflow-model/source-map/typescript, so a clean consumer install fails at bundle time. Added as a peerDependency (^2.0.0, matching how aws-cdk-lib and constructs are already declared) plus a devDependency so this repo still builds. #12 input/payload/initialState/startInput bypassed the injection controls: emitValue re-serialized JSON but passed any other text through verbatim, with none of the requireExpression checking used for runIf/shouldComplete. Reproduced the reported escape — it closed the call early, fetched to an external host with AWS_SESSION_TOKEN, and commented out the trailing syntax. Agreed this is Medium rather than High (step code is raw by design, so no privilege escalation), but it defeated the specific threat we claimed to defend: a payload in a one-line field is invisible to someone reviewing the canvas. Fixed inside emitValue rather than at the four sites, so all ELEVEN callers are covered — including httpCall's query/headers/body and the dag duplicates, which the report did not list. JSON still short-circuits; only expression text is checked. #6 timeout inference, all three: - durationCode was never read, so a 30-day dynamic wait inferred the 60s floor and the execution died mid-flight. A dynamic duration is genuinely unbounded at synth time, so rather than guess it now yields UNKNOWN_WAIT, which clamps to the one-year cap — erring long, since a short timeout kills a live execution while a long one only delays a failure. hasUnboundedWait() plus a CDK Annotation warning make that visible instead of a silent one year. - chainInvoke also contributed 0, making a chain of long-running durable functions invisible. Same treatment. - Memo poisoning confirmed. memo.set cached values computed while the cycle guard had truncated a subtree, then reused them where the edge should have counted; this shortened acyclic branches too, which is broader than the documented "loops count a single pass". Now a result is cached only when nothing beneath it was truncated. - The null parallel branch crash is fixed (guarded like analyzePermissions.ts already does); map/group bodies guarded too. #11 dag, deps and result are now reserved. A node named after any of them shadows its own binding on the defining line — a TDZ ReferenceError at runtime rather than a compile error. Folded result in as suggested, so it is no longer ad hoc. 12 new tests; cdk 256 green.
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
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.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Issue Link, if available
Description
Language SDK doesn't do automatic parsing anymore, so the testing SDK shouldn't either. Only strings are accepted in sendCallbackSuccess now.
Demo/Screenshots
Checklist
Testing
Unit Tests
Have unit tests been written for these changes? Updated
Integration Tests
Have integration tests been written for these changes? Updated
Examples
Has a new example been added for the change? (if applicable) Updated