Repository navigation
Conversation
…tPendingInterrupts
stream() and resume() accepted any conversationId with a caller-supplied
userId and never checked that the conversation belonged to that user, so
a non-owner could add turns to another user's conversation or approve
(and trust) their pending tool call. Both now reject with
ConversationNotFound unless the { userId, conversationId } record exists,
the same check deleteConversation() already used.
getPendingInterrupts() now takes the caller's userId and returns [] for a
conversation that user doesn't own.
The README End-to-End example took userId from the request body; it now
derives it from the authenticated session.
…-scoped reads and channels
Follow-up to the conversation ownership checks, applying the review:
- The ownership read is now strongly consistent. DistributedTable.get()
takes a new GetOptions { consistentRead } (maps to ConsistentRead on
AWS; the mock is always consistent), and ownsConversation() uses it,
so stream() right after createConversationId() is not rejected on AWS.
- getPendingInterrupts() without a userId throws InvalidUsage naming the
new signature instead of returning [] on the mock and an opaque
marshalling error on AWS.
- getConversation(id, userId, options?) is owner-scoped: [] for a
conversation the caller does not own, InvalidUsage without a userId.
- With persistence, a turn's chunks are published on a channel scoped to
the user who started it, and getChannel(channelId, userId) resolves the
same key, so a caller-chosen channelId can no longer publish onto, or
subscribe to, another user's live channel. inferenceOnly agents keep
unscoped channels and an optional userId.
- Tests: unit tests for each of the above plus the inferenceOnly
exemption, and a cross-user e2e in test-apps/comprehensive (user B
stream/resume/read/push/listen on user A's conversation).
- Docs: README Authorization section and End-to-End backend (adds the
getPendingInterrupts and resume handlers, uses requireAuth), DESIGN.md
for both packages, changesets, API reports.
…tion sessions
On AWS a turn without a conversationId used the caller-chosen channelId
as its AgentCore runtimeSessionId, and the AgentCore entry fell back to
that session id as the conversationId. stream({ channelId: <another
user's conversation id> }) without a conversationId therefore skipped
the ownership check and ran inside that conversation's session. The
entry now takes conversationId from the request only (toTurnPayload),
and such turns get a session derived from their user and channel, as
the local mock already behaved.
Also from review:
- getChannel(channelId, userId) now requires userId, like
getConversation() and getPendingInterrupts() (inferenceOnly agents
don't use it).
- Document the new errors (README table, stream()/resume() JSDoc),
StreamOptions.conversationId/userId/channelId and
AgentStreamResult.channelId, the inferenceOnly caveat, and the
channel-key change on upgrade; wire resume/getPendingInterrupts in the
README End-to-End frontend.
- The umbrella @aws-blocks/blocks changeset entry for the Agent change.
- e2e: a conversation-less turn by user B onto user A's id leaves A's
history unchanged.
From the second review round: - invokeTurn() scopes the channel inside its try, never reports an error on the bare caller-chosen channelId (it could be another user's scoped channel), and logs a failed error publish instead of leaving an unhandled rejection in the fire-and-forget turn. stream() and resume() scope the channel before dispatching, so an unscopable userId fails the call itself; stream() no longer leaves its channel promise unhandled. - The AgentCore invocation handler moves to an internal module (agentcore-invocation.ts, not in the export map) and is tested with an AgentCore session id, so a fallback to it as the conversationId would fail a test. - Docs: InvalidUsage only applies with persistence; mark the conversation-less AWS session change as breaking for deployed agents that relied on it; DistributedTable read-consistency wording.
From the third review round: - resume() derives the user-scoped channel before it persists any approval, so a rejected resume leaves no approval row behind. - A userId that cannot be encoded into a channel key throws a branded InvalidUsage instead of a raw URIError (which reached clients as a nameless 500); the AgentCore session hash for a turn without a conversation no longer encodes, so it never throws. - invokeTurn's JSDoc names the one case that publishes no chunk. - Changeset title and DESIGN.md wording.
…d writes From the fourth review round: - resume() scopes the channel and validates the tool context before the ownership read and before persisting approvals, so a rejected resume (including an invalid toolContextSchema context) leaves no approval row that would hide the still-pending interrupt; stream() scopes the channel before its ownership read. - Tests: the context-rejection case and the conversation-less session hash (never throws, keeps (userId, channelId) pairs apart). - JSDoc: stream()/resume() list InvalidUsage (and resume the other errors it throws); invokeTurn names both cases that publish no chunk; agent.aws.ts session-id comments describe the conversation-less case.
…g approvals Comment-only follow-up from the fifth review round: approvals are persisted after every validation and ownership check, but a dispatch failure can still reject resume() after they are written. Also list InvalidModelConfig in stream()'s @throws and reword the AgentCore session-id comment.
🦋 Changeset detectedLatest commit: f4c9d30 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…sion A turn without a conversation has no state to keep, so its AgentCore runtimeSessionId is now a fresh turn-<uuid> instead of a hash of the user and channel. No two such turns (of the same or different users) share a session, and the user id no longer flows into a hash (flagged by code scanning as a password hash).
Contributor
📖 Glossary & notes — scoring, indicators, per-metric thresholds (click to expand)
Results — PR vs
|
| Task | Template | Tests | Judge | Cost | Turns | Score | Stop reason |
|---|---|---|---|---|---|---|---|
| async-word-counter | bare | — | — | ✨ $3.38 (-$3.62) | ✨ 21 (-15) | — | endTurn |
| auth-notes | demo | — | — | ✅ $6.29 (-$0.46) | ✅ 32 (0) | — | endTurn |
| cognito-profile | auth-cognito | — | — | ✨ $9.81 (-$4.42) | ✨ 40 (-9) | — | endTurn |
| collab-presence-board | default | — | — | ✨ $2.16 (-$1.05) | ✨ 16 (-5) | — | endTurn |
| email-digest | demo | — | — | ✅ $5.3 (-$0.1) | — | endTurn | |
| file-gallery | bare | — | — | ✨ $1.83 (-$1.2) | ✨ 14 (-4) | — | endTurn |
| kb-chat-agent | demo | — | — | ✨ $18.25 (-$5.38) | ✨ 46 (-7) | — | endTurn |
| observability-api | backend | — | — | ✅ $5.17 (+$0.07) | ✅ 27 (-2) | — | endTurn |
| oidc-dsql-notes | react | — | — | ✅ $8.23 (-$0.7) | ✅ 35 (0) | — | endTurn |
| sql-kb-catalog | nextjs | — | — | ✅ $5.59 (-$0.06) | — | endTurn |
Merge verdict
✅ This bench run does not block the PR.
- ⚪ Absolute floor — BENCH_MIN_SCORE=65 set, but no cell was scored — floor skipped (conservative).
- ⚪ Regression vs
main— BENCH_MAX_REGRESSION=10 set, but no main baseline to diff — regression gate skipped.
The Δ vs base ball above is a fixed ±5-pt display band; the regression gate uses BENCH_MAX_REGRESSION. The ball is cosmetic — the gate threshold is what blocks.
Caveats & exclusions
- 🧰 Excluded (harness_error, not scored) — 10: 10× isolation_unavailable. Cells:
async-word-counter/bare,auth-notes/demo,cognito-profile/auth-cognito,collab-presence-board/default,email-digest/demo,file-gallery/bare,kb-chat-agent/demo,observability-api/backend,oidc-dsql-notes/react,sql-kb-catalog/nextjs
3 tasks done
This branch had an error being deployed
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.
#939 rebased + review fixes.
This draft carries #939's commit (d58e309, rebased onto current main) plus the fixes for the review on #939 (seven more commits). It targets
maininstead of #939's branch: #939's branch is behind main, so a PR stacked on it would also show every main commit since then. Merge this one instead of #939, or cherry-pick the seven follow-up commits onto #939's branch after rebasing it.What changes (conversation ownership checks in
@aws-blocks/bb-agent)DistributedTable.get(key, { consistentRead: true })(GetOptions, maps toConsistentRead: trueon AWS; the mock is always consistent; re-exported from@aws-blocks/blocksasDTGetOptions). The ownership check uses it, sostream()right aftercreateConversationId()is not rejected on AWS.getPendingInterrupts()without auserIdthrowsAgentErrors.InvalidUsagenaming the signature (review: blocking).getConversation(id, userId, options?)is owner-scoped (review open question 2).u/<encoded userId>/<channelId>, andgetChannel(channelId, userId)resolves the same key, so a caller-chosenchannelIdcan no longer publish onto, or subscribe to, another user's live channel.conversationIdgets its own AgentCore session on AWS. Found in this follow-up's own review: the runtime session id was the caller-chosenchannelIdand the AgentCore entry used it as the conversation, which bypassed the ownership check. The entry now takesconversationIdfrom the request only, as the mock already behaved, and each such turn gets a freshturn-<uuid>session.inferenceOnlyagents keep no owner; their channels stay unscoped. The README now says that aconversationIdpassed to one is a shared session key.Breaking changes (0.x minor, flagged for maintainer review)
The original reviewer recommended each of these:
getPendingInterrupts(conversationId, userId): positionaluserId, as in fix(bb-agent): check conversation ownership in stream, resume, and getPendingInterrupts #939 (the reviewer's suggestion block keeps it positional; it matchesdeleteConversation/listConversations/createConversationId, and TypeScript callers get a compile error that points at the fix).getConversation(id, userId, options?): new required second argument, before{ limit }.getChannel(channelId, userId): new required second argument, and the method is nowasync.inferenceOnlyagents don't use it.channelIdwithout aconversationIdto keep memory across turns loses that memory; pass aconversationId.Changesets:
@aws-blocks/bb-agentminor,@aws-blocks/bb-distributed-tableminor (consistentRead),@aws-blocks/blocksminor (re-exports Agent andDTGetOptions). At release, changesets moves bb-agent's internal range on bb-distributed-table to the new minor. No third-party dependency is added or upgraded.Review comments on #939
consistentReadoption, used byownsConversationgetPendingInterruptsinferenceOnlyexemption untestedstream()with a conversationId on an inferenceOnly agent completes)getPendingInterrupts/resumegetCurrentUserthenuser.userIdrequireAuthin all three server snippetsuserIdkept, guard addedgetConversationscopingchannelIdtest-apps/comprehensive/test/agent.test.ts"Cross-user conversation ownership": user B stream/resume/read/push/listen on user A's conversation, plus a conversation-less turn onto A's idTests
npm run build,npm run lint:deps,npm test,npm run test:e2e:local,npm run lintall exit 0.bb-test-prod-pr-1016-1-runner-...-test-app-preset-balanced-snis 64 characters, one over S3's limit, because this PR number has four digits. The bucket and agent ids are main's.Residuals
establishedpromise (the client does not re-sendsubscribe). Seen while writing the single-client e2e; not changed here.channelIdnear Realtime's 1024-byte path limit now fails a little sooner because of theu/<user>/prefix. The turn reports it in the logs instead of crashing the process.