Repository navigation
Merge main into feature/mcp-security-enhancement - #2894
Open
aws-toolkit-automation wants to merge 30 commits into
Open
aws-toolkit-automation wants to merge 30 commits into
aws-toolkit-automation wants to merge 30 commits into
Conversation
… throttle cooldown, reaped-beam drop-off (#2883) * fix(beam): cache parsed beam artifacts so discovery stops re-downloading them Beam discovery re-scans the same candidate artifacts on every sweep, and each candidate was downloaded and re-parsed every time. Measured in one 90-second window on a workspace with 32 jobs: 1980 CreateArtifactDownloadUrl completed 1230 "artifact is neither raw JSON nor a zip" parse failures 295 listBeamedRepos calls 217 CreateArtifactDownloadUrl AccessDeniedException That is ~22 download-URL creations per second, sustained. The AccessDenied errors were not confined to artifacts - ListMessages took the same error in the same window, so this starves unrelated calls, chat included. The existing bounds (BEAM_MAP_SCAN_MAX_CANDIDATES, BEAM_MAP_SCAN_BUDGET_MS) limit ONE scan and are correct, but they reset per call and so cannot limit the number of scans. Caching is what bounds the total. Artifacts are immutable - a re-upload mints a new artifactId - so a cached entry can never be stale. Keyed on artifactId, capped, and cleared wholesale on overflow rather than evicting a chosen victim; the only cost of a clear is downloading once more. A cached `null` means "these bytes are definitively not JSON", which is the case that dominated the waste (1230 of the failures). It also collapses that log line from once-per-sweep to once-per-artifact. Transient failures are deliberately NOT cached - a missing presigned URL, a network or throttle error, an AccessDenied, or a malformed zip that makes AdmZip throw. Caching one of those would permanently mark a readable artifact unreadable. Not changed here, deliberately: - No result-level cache for listBeamedRepos. Its result genuinely changes as lbvOpen flips, and making that staler risks the LBV-open timing that the Load gate depends on. With artifact downloads cached, a repeat call is a single ListArtifacts plus cache hits. - No name/extension prefilter on candidates. The code already documents why that regressed before: the beam-map is itself stored as a zip with a .zip path, so filtering on the extension skipped the beam-map entirely and fell back to zip-discovery with an empty stepId. - No cadence change on the IDE side. Measure the effect of this first. * fix(beam): correct the artifact cache - the cap was below the working set The previous commit's cache did nothing. Built and run, the flood was unchanged at ~1200 CreateArtifactDownloadUrl per minute, and the new "caching as not-JSON" line fired 14649 times in 25 minutes. Measured why, rather than guessing again: distinct artifacts touched 1043 previous cap 500 wholesale clears in 25 min 36 The cap sat below the working set, so the cache filled, cleared everything, refilled, and cleared again - discarding on every pass the entries that were about to be hit. Hit rate approximately zero. It paid the bookkeeping and delivered none of the benefit. Two changes: 1. Cap 500 -> 4000, derived from the 1043 measurement rather than chosen. Redundancy in that session was ~29x (about 30000 downloads for 1043 distinct artifacts), so a cache that actually holds the working set should remove most of the volume. 2. Evict ONE least-recently-used entry instead of clearing the map, and refresh an entry's position on a hit so the ordering is a true LRU (Map iterates in insertion order, so "oldest key first" is only correct if hits re-insert). The eviction strategy is the part that mattered. Clearing wholesale turns an under-sized cap from a partial loss into a total one: at the cap it discards every entry including the ones in active use. Single-victim eviction degrades gradually, so a workspace whose working set exceeds the cap still gets most of the benefit instead of none. The cap-reached notice is kept but latched to log once per process rather than once per eviction - that signal is what made the under-sized cap diagnosable, and per-eviction logging would itself become a flood. Still to verify by measurement, not assertion: that this actually reduces the download volume. The previous version looked correct and did nothing. * fix(beam): stop the artifact cache assuming artifacts are immutable The cache was keyed on artifactId with no expiry, justified by "a re-upload mints a new artifactId". That is asserted nowhere in the platform contract, and this file's own comments record the opposite: web-orc RE-WRITES the beam-map ("re-written on each beam; latest wins"), and the BeamArtifactId "thrash" that forced the beam-status fallback is the same observation. If a rewrite reuses the id, a permanent cache pins the FIRST beam-map for the whole LSP session — so a repo beamed later never appears in the panel until restart, and a re-beam leaves chat routed at a dead stepId. Split the policy by polarity instead, which needs no platform guarantee: negatives ("not JSON") -> cached permanently; a property of the bytes, and 1230 of the 1980 wasted fetches, so this is where the win actually is positives (parsed) -> 60s TTL; far longer than the ~13s sweep so the repeated fetches still collapse, while staleness is bounded to a minute The TTL also bounds RETENTION: parsed values are arbitrary size, so without one, customer artifact content stayed resident for the whole session. Also label the beam discovery logs with their job id. One workspace commonly holds several jobs containing the SAME repo names, so unlabelled per-repo lines read as one repo changing state when they are really N jobs interleaved — which produced a confident, wrong "lbvOpen is flapping" diagnosis that was really four jobs each reporting correctly. * fix(beam): stop skipPolling duplicating every message the user sends The skipPolling early return handed back the raw sendResult, whose sent-message id sits at `message.messageId`. The polling path below it returns `data: { sentMessage: sendResult.message }`, and the client reads `data.sentMessage.messageId` to add the id to its seen-set so its own 3s poll does not re-render the message it just sent locally. On the skipPolling path that read resolved to null, nothing was seeded into the seen-set, and the poll then treated the user's own message as newly arrived — so every beamed message appeared twice in the transcript. Return the same shape the polling path returns. Regression from the skipPolling change; only beamed sends set the flag, so only they were affected. * fix(beam): pick the live beam node, and back off when throttled Discovery matched a beamed repo by name and took the first hit. A repo beamed more than once has more than one node under that name, and the first is the oldest, so a re-beamed repo never came back - the IDE kept resolving to a step that had already finished. Matching now collects every node with that name and prefers one whose build-verification child is still live, falling back to the newest. A matched node's subtree is not searched again, so nested names cannot shadow their parent. The beam-map scan also had no reaction to being throttled: it would retry the same sweep on the next poll and keep the throttling alive. It now sits out a cooldown longer than the poll interval, so whole sweeps drop rather than being retried piecemeal. The cooldown empties the candidate list instead of returning early, because returning early would abandon beam-status discovery too and empty the tab for the whole cooldown. * fix: drop a reaped beam from the transferred list instead of showing it forever A beam whose sub-agent never booted gets reaped by the web orchestrator: the instance is stopped and the repo node marked terminal. The IDE kept showing it, because the only signal read was the Local Build Verification child - and a reaped beam has none, which is indistinguishable from a fresh transfer whose HITL has not been created yet. Read the repo node's own status to separate them. Terminal with no LBV child means the beam ended without ever opening LBV, so it is closed. Safe on re-beam: the orchestrator re-drives that node to IN_PROGRESS before reusing it, so a re-beamed repo reads live again. The user is otherwise left waiting on a repo that will never build - the re-beam notice goes to the job owner, not to whoever is in the IDE. * test: cover the reaped-beam gate in both directions The terminal-repo-node branch had no behavioural coverage. Deleting it or inverting it both left the suite green, so a passing run could not tell a working gate from an unreachable one. Four cases, mutation-verified: removing the gate fails two, over-gating fails the third, and the fourth pins that an LBV child still decides when one exists.
* fix(amazonq): stream large ATX artifact downloads to disk
downloadArtifactToPath buffered the whole artifact into memory via
got.get(responseType:'buffer') under timeout:{request:30000}. The
'request' timeout caps the ENTIRE request including body transfer, so a
large artifact (e.g. Transformation_Report.html) that takes >30s to
download aborts with a TimeoutError. That error was swallowed by the
catch and surfaced only as a generic download failure — matching the
customer report where CreateArtifactDownloadUrl succeeds but the report
never downloads (V2381716118).
Stream the presigned URL straight to disk with got.stream + pipeline,
and switch to inactivity-based timeouts (response 30s to headers, socket
60s idle) so a large-but-healthy download is no longer capped by wall
clock, and no longer double-buffered in memory. Add success/error
logging for diagnosability.
* fix(amazonq): log all downloadArtifactToPath failures with diagnostics
Every failure branch now logs clearly: the previously-silent missing-URL
branch logs an error, and the catch records the got error code, timeout
phase, HTTP status, bytes transferred before failure, and full stack.
* feat(chat-client): persist deprecation notice dismissal * fix(chat-client): replace agentic feature card * fix(chat-client): show deprecation notice once per instance * fix(chat-client): finalize deprecation notice * fix(chat-client): preserve deprecation notice on chat options * fix(chat-client): show deprecation notice until acknowledged * test(chat-client): document JSDOM Node global
…xists (#2886) * fix(amazonq): forward the missing-packages HITL when a plan already exists handleAwaitingHumanInput routes to handleExecutionPhaseHitl once a plan is present, and that path only handles step-level ({stepId}-review) HITLs - it never forwards HitlTag or MissingPackageJsonPath. A missing-packages HITL raised after planning was therefore dropped on the way to the IDE, which could not show the package-upload dialog. Only the planning path forwarded those fields, so the defect appeared exactly on longer jobs where the plan already existed. Return the HITL directly when it is reported, mirroring the existing local-build-verification early-exit, and log the resolved json path. Client-side handling of this HITL is P516058803. * test(amazonq): cover missing-packages HITL passthrough when a plan exists Verified against the unfixed handler: the two plan-exists cases fail without the passthrough (the tag and MissingPackageJsonPath are dropped by handleExecutionPhaseHitl) and pass with it. The no-plan case passes either way, confirming the planning path was never affected - which matches the reported job, where the language server forwarded the HITL correctly.
…fact, not the job objective (V2381727290) (#2887) * fix(amazonq): keep interactive_mode when a checkpoint sync omits it (V2381727290) checkpoint-settings.json is full state: it is uploaded as a HITL artifact and the agent applies whatever it contains. The agent reads a missing interactive_mode as "use the default" (interactive) rather than "leave the mode alone", so any writer that omits the key silently reverts the user's mode - absent is not unchanged. setCheckpoints only attached interactive_mode when the caller supplied one, which was safe while the IDE attached a mode to every periodic checkpoint sync. It is not safe once a caller legitimately has no mode to assert: on a gamma job the user switched to autonomous at 07:16:57Z, the agent picked that up and ran autonomously from 07:18:34Z, and a later checkpoints-only sync uploaded a settings artifact with no interactive_mode - flipping the agent back to interactive at 07:22:52Z. Carry the last written mode forward when the caller does not supply one, so a checkpoints-only sync preserves the mode instead of dropping it. An explicit mode still wins. Unrecognised or corrupt persisted values are ignored rather than echoed to the agent, and reading them never fails the sync. This is the writer-side half. The agent treating absence as "keep current" is the matching robustness fix and is tracked separately, as is the read path: the LSP still restores the mode from objective.interactive_mode, which is the immutable job-creation payload, so a mid-job switch cannot survive an IDE restart until the service exposes the effective mode. Deliberately not sourcing that from the local file this change reads - it would go stale whenever the mode is changed outside this IDE. * fix(amazonq): restore the mode from the settings artifact, not the objective (V2381727290) The mode reported to the IDE after a restart came from objective.interactive_mode. The objective is the payload the job was CREATED with and is never rewritten, so a mid-job switch was silently discarded: reopening the IDE always came back in the mode the job started in. Switch to autonomous, reopen, and the hub showed interactive again. A mode change is written to the checkpoint-settings HITL artifact instead, and every client that changes the mode writes it - this IDE, another IDE, or the web UI - so it is both current and shared. Read the mode from there and keep the objective as the fallback for a job whose mode was never changed and so has no artifact yet. An artifact that carries no usable interactive_mode also falls back rather than guessing. The read reuses the existing download path and stays one-shot, gated on the same cachedInteractiveMode latch as before, so it costs one extra list plus download per job on the first poll rather than one per poll. Worth recording why the local checkpoint-settings.json this handler already writes is NOT used as the source: it only sees switches made from this IDE on this machine, so preferring it would report a stale mode whenever the mode was changed elsewhere - trading a known bug for a harder cross-client one. * fix(amazonq): keep the cached mode in step with an asserted switch Two gaps found reviewing the earlier commits on this branch. setCheckpoints never updated cachedInteractiveMode, so after a mid-job switch the cache still held the job-start value - and that cache is what getTransformInfo reports back to the IDE on every poll. Nothing broke only because the IDE applies the restored mode once and then ignores later values; the correctness of a switch should not rest on that latch. Update the cache when a mode is asserted. The carry-forward then reads that cache in preference to the local checkpoint-settings.json. The file only records switches made from this machine, so a mode changed from the web UI would have been overwritten with a stale local value by the next checkpoint sync - turning the carry-forward into the very kind of silent revert it was added to prevent. The cache is seeded from the settings artifact, which every client writes, so it does not have that blind spot. The file stays as the fallback for a process that has not yet resolved a mode. * fix(amazonq): key the interactive-mode cache by job (review feedback) The cache was a single field on a handler that outlives any job, reset only on a terminal status or a new startTransform. That used to be a display bug at worst. With the carry-forward added earlier on this branch it is not: a checkpoint-only sync writes the cached mode into the job's settings artifact, and the agent treats that artifact as authoritative - so with two jobs open, the first job's mode could be written into the second job's artifact and change how it runs. Keyed by job id now, and the job-start write moved after createJob so the id exists to key on. clearJobCache drops only the entry for the job it was called with; every caller already passes one. Also records where each cached mode came from, so the carry-forward log names it. Carrying forward a mode that came from the job-creation objective asserts the job's starting mode rather than a real choice, which is only correct while no switch has happened - and that is the known-limitation path where findCheckpointSettingsHitl misses, which was previously indistinguishable in a log. Two tests cover the reported scenario: a sync for job-2 does not write job-1's cached mode, and each job keeps its own asserted mode. Reverting the job scoping fails the first with job-1's mode present in job-2's artifact. * docs: trim the mode-cache comments Comment-only. Condenses the carry-forward and cache-declaration comments, and records the one known limitation in a line rather than a paragraph: the cache is resolved once per job, so a switch made elsewhere afterwards is overwritten by the next sync.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: aws-toolkit-automation <>
The filesystem tools derived the path they operate on differently from the path the workspace-containment check evaluates. `requiresPathAcceptance` resolves symlinks at every segment via `resolveSymlinkAwarePath`, while `fsWrite`, `fsReplace`, `fsRead`, `fileSearch`, and `listDirectory` called `sanitize()`, which only expands `~` and makes the path absolute. The unresolved path was then handed to `writeFile`/`readFile`/the directory walk, so the operating system dereferenced the link when it opened the file rather than the tool operating on the target it had evaluated. Add `resolveCanonicalPath()` to `toolShared` (sanitize, then resolve symlinks) and use it in `requiresPathAcceptance` and all five tools, so the containment check and the operation derive the path the same way and each tool acts on a concrete resolved target it computed. `sanitize()` is composed with the resolver rather than replaced by it: `resolveSymlinkAwarePath` does not expand `~`, so calling it alone would drop tilde expansion at these call sites.
The `after`/`afterEach` hooks called `tempFolder.delete()` and `tempFolder.clear()` without awaiting them, so cleanup from one test could still be removing files while the next test wrote its fixtures and read them back. `reads multiple files` failed with ENOENT on a file it had just written once the tool gained additional await points before its read. The four sibling suites (fsWrite, fsReplace, listDirectory, fileSearch) already await these calls; this makes fsRead consistent with them.
`handleCreate` reports the path it wrote to, which is now the resolved path, so the assertion must compare against the canonical form rather than the path the test passed in. On Windows `os.tmpdir()` yields an 8.3 short name (RUNNER~1) that resolution expands to its long form (runneradmin), which failed the deep-equal on that platform only. Derives the expected value with `fs.realpath` rather than the tool's own helper, so the assertion stays an independent check of the reported path.
The workspace-boundary check treats "lstat does not report a symlink" as "this path refers only to where it appears to." That holds for symbolic links, which `resolveSymlinkAwarePath` follows, but not for a file carrying more than one name: `lstat` reports such a name as an ordinary file, so there is nothing for the resolver to follow and the canonical path is the in-workspace name. The file's other names stay invisible to the check, and an in-place write changes the contents under all of them — including names outside the workspace. Those other names cannot be recovered from the path. An inode has no location and there is no reverse inode-to-paths lookup, so the check cannot prove they are inside the workspace; the link count is the only signal available. `hasAdditionalHardLinks` reports it, excluding directories, whose count is always above one, and paths that do not exist yet. `fsWrite` and `fsReplace` opt in via `flagMultiplyLinkedFiles`, so such a write now prompts with an explanation instead of proceeding silently. Approval still short-circuits first, so an approved path is not re-prompted. Read and listing tools are unchanged: their target has a legitimate name inside the workspace, and package managers that hard link into a shared store would make a read-side prompt frequent.
The workspace-boundary check treated "lstat does not report a symlink" as "this path refers only to where it appears to." That holds for symbolic links, which `resolveSymlinkAwarePath` follows, but not for a file carrying more than one name: `lstat` reports such a name as an ordinary file, so there is nothing for the resolver to follow and the canonical path is the in-workspace name. The file's other names stay invisible to the check, so reading the path returns data that also lives under them and an in-place write changes it under all of them, including names outside the workspace. Those other names cannot be recovered from the path. An inode has no location and there is no reverse inode-to-paths lookup, so the check cannot prove they are inside the workspace; the link count is the only signal available. `hasAdditionalHardLinks` reports it, excluding directories, whose count is always above one, and paths that do not exist yet. `fsWrite`, `fsReplace`, and `fsRead` opt in via `flagMultiplyLinkedFiles`, which also states how the tool accesses the file so the prompt describes the right consequence. `listDirectory` and `fileSearch` take a directory, whose link count carries no such meaning. Three things were needed to make that prompt trustworthy: The session recorded every path a tool touched, not the paths the user allowed, and `requiresPathAcceptance` short-circuits on a recorded path. A path that was harmless on first use was therefore exempt from every later check, so touching a file before it gained a second name skipped the new prompt entirely. Paths are now recorded only once the user has been asked and has allowed, which is what the surrounding comment already claimed. An allowed path is compared in canonical form, so the raw input was recorded under a name the check never looks up whenever the path reaches the file through a symlinked ancestor — a symlinked home directory, or macOS /tmp. Both forms are now recorded, otherwise the user is asked again on every call. The `warning` explaining why acceptance is needed was used only as a flag to prepend a blank line, never shown. It is now part of the confirmation body, and the filesystem prompts no longer describe a multiply linked path as being outside the workspace, which it is not.
fix: harden tool path handling
* fix: enforce CodeReview artifact workspace boundaries * fix: improve CodeReview portability and scan efficiency Use a real empty temporary Git config instead of the Windows device path. Full reviews run no Git; diff reviews derive changed-file counts from the same per-file diff. Preserve submitted folder paths in archive entries while reading checked canonical paths. Add 16 regression tests for full/diff scans, archive path mapping, invalid workspace roots, pre-read failures, and nested folder traversal. Focused suites: 159 passing. Local c8 reports 437/437 mapped added lines and 78/78 added-line branch arcs covered; this is not a published CI coverage result. Compilation, formatting, and full-diff review passed. Windows CI must validate the portability fix. * test: match canonical paths in boundary failure stubs
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This merges the released changes for 1.81.0 into main.
… S3 RequestTimeout (#2926) * making artifact upload process a worker task * fix(amazonq): upload transform artifact from a worker thread to avoid S3 RequestTimeout The artifact upload ran on the language server's main thread, which it shares with the workspace indexing library. The 30-minute index refresh blocks that thread for up to a minute or more on large workspaces; during that time no upload bytes are sent, S3 closes the PUT with 400 RequestTimeout, and StartTransform fails with Failed to upload ZIP file to S3. Move the PUT into a worker_threads worker so it keeps streaming while the main thread is busy. uploadArtifact's signature and return contract are unchanged.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: aws-toolkit-automation <>
…n tests GitHub retired the macos-13 hosted runner image on 2025-12-04. Jobs that request it are never picked up: the mac-x64 Agent Server Tests job sits queued with no steps until the workflow's 24h limit cancels it. macos-15-intel is GitHub's supported Intel (x86_64) macOS label, which keeps the mac-x64 target covered.
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
9 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.
Automatic merge failed
Command line hint
To perform the merge from the command line, you could do something like the following (where "origin" is the name of the remote in your local git repo):