Repository navigation
Merge main into feature/qdev-signup-message - #2826
Open
aws-toolkit-automation wants to merge 48 commits into
Open
aws-toolkit-automation wants to merge 48 commits into
aws-toolkit-automation wants to merge 48 commits into
Conversation
…d beam discovery/download (#2798) * feat: scope local build verification to the loaded beamed repo and add beam discovery/download * fix: only attach StepId to local-build-verification HITL when present The beam LBV early-return always included a StepId field, so a HITL with no stepId returned { HitlTag, TaskId, StepId: undefined }. The existing unit test asserts the return deep-equals { HitlTag, TaskId }, which the extra key broke. Attach StepId only when defined so the non-beam return shape is unchanged; beam jobs still carry the stepId. * fix: address PR review — escape jobId, validate beam-map, guard zip-slip, tests Addresses inline review comments on PR #2798: - Regex injection/ReDoS: escape parentJobId before interpolating into new RegExp() in listBeamedRepos. - Unvalidated beam-map JSON: shape-validate downloadJsonArtifact result (only objects yielding a non-empty repo name survive; malformed/null entries rejected + logged) before it drives artifact selection. - Zip-slip: extractAllEntriesTo now resolves each entry against the extraction root and skips (logs) entries that escape via ../ or absolute paths. downloadBeamArtifact adds defensive destDir validation (rejects non-absolute, ..-containing, or empty paths). - Backoff jitter: throttle retries use base * (0.5 + Math.random()). - Typed contract: new BeamMapRepo / BeamedRepoInfo interfaces; listBeamedRepos returns Promise<BeamedRepoInfo[]>. Removes `any` from the beam discovery path. - Bounded candidate scan: cap at BEAM_MAP_SCAN_MAX_CANDIDATES = 25 and BEAM_MAP_SCAN_BUDGET_MS = 15000 to keep listBeamedRepos responsive on artifact-heavy jobs. - Shared scoped-HITL helper: selectScopedLbvHitl centralizes the scoped-LBV selection previously duplicated between getHitlAgentArtifact and the EXECUTING path. - Central getStepId helper: single source of truth for the stepId ?? planStepId ?? parentStepId coalescing across all call sites. Test coverage: - New Beam to IDE unit suite for getStepId, normalizeBeamRepo, and listBeamedRepos (empty / beam-map / beam-status fallback / hash-suffix / regex-edge / send-failure), plus downloadBeamArtifact destDir rejection and a zip-slip regression test in utils.test.ts. Deferred as tracked follow-ups (reviewer flagged non-blocking): - Silent error swallowing → {repos, hadErrors}: return-shape change needs coordinated IDE-client update. - Extract BeamRepoService: ~700-line refactor; hold for a dedicated PR to avoid pre-release regression risk on the shared handler.
… client (#2800) * fix(amazonq): pass notification feature to the Q service manager (#2796) The access-blocked notification added in #2794 never reached the client. AmazonQServiceServerFactory destructures the features it forwards to the service manager, and notification was not among them, so features.notification was always undefined, the guard in serviceFactory never passed, onAccessBlocked was never assigned, and the notifier could not run. notification is optional on QServiceManagerFeatures so that existing constructions and test fixtures keep compiling. That is also why omitting it here did not fail the build -- it silently disabled client-facing reporting instead. Noted at the call site so the next person adding a feature there does not repeat it. Also set a stable id on the notification. Clients need to recognise it without inspecting its text: the message is the service's own copy and is expected to change, and FEATURE_NOT_SUPPORTED is reused across several RTS gates so the reason alone does not identify this one. Both IDE clients already prefer the id when present and fall back to matching the title only because the released server does not send one yet. Verified: tsc clean, prettier clean, 6/6 notifier tests pass, and a server bundle built from this branch contains the wiring where a bundle from the previous head did not. * fix(amazonq): declare serverInfo so notifications reach the client (#2797) * fix(amazonq): declare serverInfo so notifications reach the client The access-blocked notification still never reached the client after #2796. The runtime only constructs a notification router for servers that declare serverInfo: if (initializeResult?.serverInfo) { this.notificationRouter = new RouterByServerName(initializeResult.serverInfo.name, ...) } AmazonQServiceServer returned only capabilities and awsServerCapabilities, so the router was never built and notification.showNotification() logged "Notifications are not supported: serverInfo is not defined" and dropped the notification. Observed in VS Code: the block was detected and logged, then silently discarded. This is the last piece. With #2794 (detect), #2796 (wire) and this change (deliver), a blocked identity produces a notification the client can act on. Added a regression test, because the failure mode is silent: nothing throws and only a debug line marks the loss. The test asserts the exact name, which is deliberate -- the name is encoded into the id of every notification the client echoes back, so renaming it strands followups for notifications already on screen. Note: amazonQServer.test.ts has one pre-existing failure on this branch, "hooks onUpdateConfiguration handler to LSP server", present before this change (6 passing/1 failing before, 7 passing/1 failing after). Left alone as unrelated. * fix(amazonq): observe access-blocked on the streaming client too The observer added in #2794 was only on the token client. Chat runs through the streaming client, so the one surface where a blocked identity actually shows up to the user was the one place nothing was watching. Detection happened to work anyway because the gate denies every operation and the A/B config fetch goes through the token client moments after credentials arrive -- but that is incidental, not a guarantee. Mirrors the token client exactly: middleware on the outermost initialize step so it fires once per operation after retries are exhausted, the observer is called inside its own try/catch, and the error is always rethrown so callers behave as before. The notifier is now created once per service generation and shared by both clients rather than created per client. The notifier dedupes per instance, so sharing is what keeps a blocked identity to a single notification no matter which client sees it first. It is cleared by resetCodewhispererService, so signing out and back in with another blocked identity notifies again instead of being suppressed. Scoped to StreamingClientServiceToken. The IAM variant serves a different surface and the gate only denies Builder ID, which is bearer-token only. Pre-existing failures on this branch, unchanged by this commit: utils.test.ts 11 failing (89 passing) and amazonQServer.test.ts 1 failing, both identical before and after. * fix(amazonq): give the IAM and token servers distinct serverInfo names (#2799) The serverInfo added in #2797 used one hardcoded name, but AmazonQServiceServerFactory is instantiated twice -- AmazonQServiceServerIAM and AmazonQServiceServerToken -- and runtimes including agent-standalone register both. Two servers reporting the same name makes lspRouter reject initialize outright: Duplicate servers defined: AWS Language Server for Amazon Q Developer That fails the whole language server, not just the duplicate. Observed in VS Code as: Failed to start downloaded LSP, falling back to bundled LSP: Duplicate servers defined: AWS Language Server for Amazon Q Developer The client then silently ran its bundled server instead, so Q appeared to work while none of the access-blocked reporting existed, with only a client-side warning to show for it. serverName is now a required parameter rather than a shared constant, since a default is precisely what let two instantiations collide. The two names are exported so the uniqueness is assertable, and they must stay stable: the name is encoded into the id of every notification the client echoes back. Added a regression test on the distinctness. Verified it bites -- reintroducing the collision gives 7 passing/2 failing, the fix gives 8 passing/1 failing. No existing test registers two servers from one runtime, which is why this reached a release. Pre-existing failure on this branch, unchanged: amazonQServer.test.ts "hooks onUpdateConfiguration handler to LSP server". * test(amazonq): address review findings on the access-blocked observer (#2801) Three review follow-ups, no behaviour change for users. Name the streaming client's middleware, matching the token client. Without a name a second registration stacks another observer rather than replacing the first, which would report the same block twice, and the middleware is anonymous in SDK stack introspection. Assert the server-name uniqueness against the real exported servers rather than the two constants. Comparing constants cannot catch the same name being passed to both factory calls, which is the mistake that actually shipped. Verified the test bites: making the names identical fails it (7 passing/2 failing vs 8/1). Add two tests for the streaming observer. They assert the wiring rather than the callback because the existing harness stubs CodeWhispererStreaming.prototype.sendMessage, which bypasses the middleware stack entirely -- a behavioural test there would pass even if the middleware did not exist. shared group: 337 passing / 45 failing, against 334 / 45 before, so the 3 new tests and no new failures.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: aws-toolkit-automation <>
…2838) The repo moved from aws/language-servers to Amazon-Q-Developer/language-servers (P490405625). CODEOWNERS still referenced @aws/flare. Team grants do not follow a repo across orgs, so GitHub cannot resolve that team and reviewer assignment is broken while main still requires code owner review. Point it at @Amazon-Q-Developer/sync-team-dae-production-eng-team, which has push access on the repo. The repository.url fields in all nine workspace package.json files also still referenced the old org. Publishing uses OIDC trusted publishing, which validates repository.url against the OIDC claims of the building repository and generates provenance from them, so stale URLs fail the publish step.
The token service issues its first service call from its constructor, and the service is built lazily by whichever consumer asks first. That consumer is the chat webview finishing boot, measured at ~0.66s after the credentials landed. Nothing contacts the service in that window, so a rejected identity cannot be discovered and the user sits in front of a chat view that is never going to work. Uses onCredentialsUpdated, added in language-server-runtimes 0.3.21, to build the services as soon as bearer credentials arrive. The dead code this replaces -- an `in` check plus an `any` cast, added in #2521 -- was reaching for this event before the runtime had it, so it never fired. Reuses handleSsoConnectionChange rather than constructing anything, so this changes only when initialization happens, not how: - Idempotent. createCodewhispererServiceInstances records the connection type, so the consumer's later call short-circuits instead of resetting live services. This matters beyond tidiness: the event fires on every token refresh, and rebuilding would drop in-flight requests and replace the service that observers are attached to. - IdC with developer profiles still stops at PENDING_Q_PROFILE and builds nothing, so no request is made before a profile is chosen. - IAM updates are ignored. - Failures are logged, not thrown: this runs inside the client's credentials request, and the lazy path still runs for whoever asks first, so a failure costs the head start only. Checked for regressions rather than assumed: the opt-out header defaults to the conservative value and configuration is read at onInitialized, long before sign-in, so an earlier request cannot leak content for a user who opted out. getConnectionType is accurate because the runtime awaits connection metadata before storing credentials. One behaviour change worth noting: ProfileStatusMonitor.emitAuthSuccess, and therefore MCP registry sync, now fires ~0.66s earlier. Tests: AmazonQTokenServiceManager 40 passing/3 failing against 35/3 on main; shared group 342/45 against 337/45. The 3 and 45 are pre-existing and unchanged.
…em silently (#2841) When a tool-use content block is streamed but the response ends before its terminating `stop` event (e.g. the output-token limit is reached mid tool-input), the tool-use input is never JSON-parsed and no error is recorded, so the parser reports success. The agentic loop keeps only stopped tool uses as pending, so the unterminated one is filtered out, the turn is reported as Succeeded, and the loop breaks -- the tool never runs and the user sees no error or retry. Add AgenticChatEventParser.finalize(), called once the response stream is fully consumed: any tool use still lacking a stop is marked stopped and given an incomplete-input error (reusing the malformed-JSON prefix) so it survives the pending-tool-use filter and is routed into the existing recovery path, which re-prompts the model to split the work into smaller tool uses. User cancellation is unaffected (aborts throw before finalize runs).
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* fix: scope and forward stepId for the planning-branch lbv hitl The AWAITING_HUMAN_INPUT path already scopes the local-build-verification HITL to the loaded beamed repo and forwards its plan-step id, but the PLANNING branch did neither: it picked the first LBV HITL in the list and returned it without StepInformation. On a multi-repo beam where the job sits in PLANNING at build time, that surfaces a sibling's (or untagged) HITL, so the IDE scope guard cannot confirm ownership and defers the build indefinitely. Bring the PLANNING branch to parity with AWAITING_HUMAN_INPUT: - prefer the in-scope LBV via selectScopedLbvHitl (no-op when scope is empty, so non-beam behavior is unchanged) - forward StepInformation.StepId when present Verified live on a 3-repo beam: each loaded repo now builds and siblings are correctly deferred. * fix: suppress out-of-scope sibling LBVs in the planning branch Brings the PLANNING branch of getTransformInfo to full parity with the EXECUTING / getHitlAgentArtifact paths for multi-repo beam. It already preferred the loaded repo's in-scope LBV and forwarded its stepId, but it did not suppress out-of-scope sibling LBVs: when scope was set with no in-scope LBV it could still surface a sibling's LBV HITL to the IDE. Mirror the sibling branches: - when scope is set and every pending HITL is a sibling's LBV, return plan-only (surface nothing) - otherwise drop out-of-scope LBVs from the fallback pool so a sibling's LBV can't be picked in a mixed pending set Not a live false-green (the IDE scope guard already rejects a HITL whose stepId is out of the loaded subtree) — this restores the LSP-side layer so all three branches behave identically. Non-beam is byte-identical: the new logic is gated on a non-empty beam scope. * fix: harden beam LSP stepId coalescing and tidy beam docs - getStepId uses || so an empty-string id falls through to the next spelling (was ?? which let an empty id defeat coalescing and scope matching) - move the normalizeBeamRepo JSDoc onto normalizeBeamRepo (was stranded above getStepId) * test: cover beam LBV planning-scope and getStepId coalescing - PLANNING branch: a sibling repo's out-of-scope LBV is not surfaced - getStepId: empty stepId falls through to planStepId/parentStepId (|| not ??)
…es in telemetry (#2847) * fix(amazonq): only retry tool-use streams that were genuinely truncated Treating every response stream that ends without a terminating tool-use `stop` event as a truncated tool input is too broad. A stream also ends without `stop` when the request is aborted (response-processing timeout or cancellation), and when the model announced a tool use but streamed no input at all. Reporting those as failures and re-prompting the model produces failed intermediate stream events for turns that were not broken, and can re-run the agent loop without making progress. - finalize() now only reports an incomplete tool input when partial input was actually received and the request was not aborted; other unterminated tool uses are left unstopped and filtered out downstream, as before. - The abort state is passed into finalize() from the response processor. - Consecutive incomplete tool-use retries are now bounded (MAX_INCOMPLETE_TOOL_USE_RETRIES). On exceeding the limit the agent loop stops and surfaces an actionable error instead of retrying indefinitely. Genuine truncation (partial input present, request not aborted) still routes into the existing recovery path and is retried. * fix(amazonq): allow 3 incomplete tool-use retries before giving up * feat(amazonq): classify incomplete tool-use retries in invokeLLM telemetry A response stream whose tool-use input is cut off is retried inside the agent loop and usually recovers within the same user turn. Every one of those iterations emits amazonq_invokeLLM with result='Failed', so a per-call success rate built on that metric drops even though the user was unaffected. Report a `reason` alongside the existing result so the two cases can be told apart downstream: INCOMPLETE_TOOL_USE_RETRYING retry budget remains; transient, recovers INCOMPLETE_TOOL_USE_EXHAUSTED retries used up; the user sees an error The classification is computed before the emit. Because the retry budget is bounded, whether this iteration will be retried is already known at that point, so no post-hoc correlation is needed. result stays 'Failed' in both cases, so raw failure counts are unchanged and remain available for diagnostics. Consumers can now exclude the transient class from success-rate calculations while still counting the terminal give-up. The reason values are consumed by ToolkitTelemetryLambda to emit a separate EMF counter; renaming them requires updating that transform first. Retry behaviour is unchanged: incrementing the counter before the emit and testing `count <= MAX_INCOMPLETE_TOOL_USE_RETRIES` preserves the existing off-by-one, so 3 retries still follow the initial failure. * chore(amazonq): describe the telemetry reason consumer generically The do-not-rename note on the reason constants pointed at a specific internal consumer by name. Describe it as a downstream metrics pipeline instead: the warning is what matters to anyone editing these values, and the constants are part of a contract rather than a link to one particular implementation. No functional change.
Co-authored-by: aws-toolkit-automation <>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Laxman Reddy <141967714+laileni-aws@users.noreply.github.com>
Co-authored-by: aws-toolkit-automation <>
…bvPending (#2849) - listBeamedRepos: derive repo name from both subdir (<repo>_<hash>/file.zip) and flat (<repo>_<hash>[_suffix].zip) transformed-source shapes, so a flat-named repo is no longer dropped from the beamed list. - Add optional Lightweight param: the IDE poll-refresh sets it to skip the beam-map download scan (throttle-safe); the IDE preserves stepId across lightweight ticks. - Add IsLbvPending (beamed but LBV HITL not created yet) via isRepoLbvHitlPending, so the IDE gates Load (IsLbvOpen && !IsLbvPending) until the HITL exists.
getTransformInfo already resolves the interactive mode from the job objective (cachedInteractiveMode) but never returned it. Add the InteractiveMode field to AtxGetTransformInfoResponse and populate it at the getTransformInfo wrapper so the IDE can restore the correct mode after a restart instead of trusting its local settings store. Paired with the IDE change in aws-toolkit-visual-studio-staging.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: aws-toolkit-automation <>
The prerelease workflow interpolated github.event values directly into inline `run:` script bodies. Because GitHub substitutes those expressions textually before the shell parses the script, a crafted branch name or workflow input could terminate the surrounding quoting and run arbitrary commands on the runner. Move each untrusted value (inputs.tag_name, workflow_run.head_branch, workflow_run.head_sha) into a step-level `env:` block and reference it as a shell variable, so the value is always treated as data. This matches the pattern the create-release job in this workflow already uses. No behavior change: tag and prerelease names are unchanged for main, feature/*, and release/agentic/* branches, and unsupported branches still fail the same way.
…eps (#2857) * feat(amazonq): render unit-test-generation substeps as status-only Mark the direct substeps of the "Generate Unit Tests" step (plan / generate / merge / coverage) as status-only so the IDE renders them as progress-only rows, without a checkpoint toggle, "View Results" button, or checkpoint checkbox. The step data returned by the service has no machine-readable step label, so the flag is assigned structurally during plan-tree assembly based on the parent step's name rather than a label prefix. The parent step keeps its normal checkpoint affordance. * feat: forward generate_unit_tests choice into ATX job objective (#2855) Include GenerateUnitTests in the CreateJob objective only when it is a real boolean, so clients that omit it keep legacy behavior and an explicit false stays a decline. Matches shipped backend contract CR-298620129. Adds 4 unit tests. --------- Co-authored-by: Meha Sharma <mehishar@amazon.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: aws-toolkit-automation <>
Bind the customVersion and versionIncrement workflow_dispatch inputs to step-level environment variables instead of interpolating them into the inline shell script, so the values are passed as data rather than expanded into the script source.
… 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.
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
11 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):