Release/1.17.0 - #912
Merged
Merged
Conversation
…rsion-snapshots-store feat(marketplace): serve immutable snapshots from the store (PR-2)
PR-3 of the agent version-snapshots epic. This is the part that makes the
feature a control rather than a display fix: a user who pinned an approved
Agent now runs the reviewed configuration, not whatever the author's draft
says today.
The invocation change is one line at chat/routes.py, because PR-1 built the
round trip for it — a version deserializes back into the same `Assistant`
that `resolve_agent_invocation` already takes, so binding resolution, the
system prompt and the harness are all untouched:
assistant, resolved_version = await resolve_invocation_agent(assistant, user_id)
- **`version_resolution.py`** holds the policy: the published snapshot for
everyone, the draft for the owner, the live record when nothing is
published. Separate from `versions.py` (pure) and `version_repository.py`
(persistence) because deciding which version a person runs is policy, and
policy reads better as one short function with the whole table in view.
- **Owner identity, not edit access.** An editor can change an Agent's
instructions but does not get to *run* the unpublished result — otherwise a
share grant is a way around review.
- **A missing published snapshot raises** (503 at the route) rather than
falling back to the draft. The fallback would serve unreviewed instructions
to a pinned user at exactly the moment something is already wrong. The owner
is unaffected: they never read the version, so a broken snapshot cannot lock
them out of their own Agent.
Purely additive: three files, and no existing behavior changes except the one
swap above.
**Deliberately not implementing spec §4.2 (version in the agent cache key).**
The spec says promoting a version would keep serving the old system prompt
from a warm agent. That is not true in this codebase: the cache key is built
from construction *values*, and everything a version changes about behavior
already reaches it — instructions via `system_prompt`, tool bindings via
`enabled_tools`, skills via `skills_hash`/`agent_type`, the model via
`model_id`, and a memory binding by skipping the cache entirely. Promoting a
version already misses.
Adding the number would buy no discrimination and would cost real safety. The
resume path rebuilds its cache key from `PausedTurnSnapshot`, so a new key
element the snapshot did not carry orphans the paused agent — an OAuth-consent
or tool-approval pause on any published Agent would fail to resume with "must
resume from interrupt". `service.py` warns about precisely that desync. An
earlier revision of this PR added the key element, hit that bug, and threaded
`agent_version` through the snapshot and `stream_coordinator` to fix it; the
honest fix was to not add the element. The reasoning is recorded inline so the
next reader of §4.2 does not re-introduce it.
Version snapshots also *improve* prompt-cache stability, which is the opposite
of the risk §4.2 implies: a published Agent's system prompt now changes only
at approval, where before it changed on every author save mid-conversation.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Doc only. PR-1 (#784) and PR-2 (#787) are merged and PR-3 (#789) is open, and building them proved parts of the spec wrong. Corrected inline with the original claim preserved, rather than silently rewritten — a spec that quietly agrees with the code teaches nobody why. **§4.2 was wrong and is the substantive fix.** It said the agent cache key "must include the resolved version, or promoting a new version will keep serving the old system prompt from a warm agent." Not true here: `_agent_cache` keys on construction *values*, and everything a version changes about behavior already reaches the key — instructions via `system_prompt`, tool bindings via `enabled_tools`, skills via `skills_hash`/`agent_type`, the model via `model_id`, a memory binding by skipping the cache entirely. A promotion already misses. Acting on it also introduces a bug: the resume path rebuilds its key from `PausedTurnSnapshot`, so a new key element the snapshot does not carry orphans the paused agent and breaks OAuth-consent / tool-approval resumes. PR-3 added the element, hit that, and threaded `agent_version` through three more files to fix it before reverting. The section now says do not re-implement it. The section also had the risk backwards: snapshots *improve* Bedrock prompt-cache stability, since a published Agent's prompt now changes only at approval instead of on every author save mid-conversation. **§3.3** — real key prefixes (`AST#`/`METADATA`, not `AGENT#`/`PROFILE`); `submittedVersion`; placement lives in the index key so an immutable snapshot is never rewritten on recategorization; and the fail-closed write ordering that replaced the atomicity lost when the index moved off the Agent row. **§7** — per-PR status, the note that D13 admin edits had to cut a version (§6.2's first option was not optional once the store renders snapshots), and a warning that `develop` is currently in the PR-2-without-PR-3 half-state. **§8** — two new open items: making the fail-closed ordering structural with a transaction, and the pre-existing MCP-app dispatch call sites that build agents from `input_data` rather than the resolved assistant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rsion-snapshots-invocation feat(agents): run the approved snapshot, not the author's draft (PR-3)
…sion-snapshots-spec docs(marketplace): reconcile version-snapshots spec with what shipped
PR-4 of the agent version-snapshots epic (§5). Closes the last unilateral
removal path: an author could pull an approved Agent out of the store, or
hard-delete it outright, with no admin ever seeing it.
D2 makes publication stop for a human. Un-publication did not — `published →
private` was in the author's own hands, and `delete_assistant` guarded on
ownership alone. Both are now admin-visible acts.
**`withdrawal_requested` is a LIVE state, and that is the crux.** The author
asks; the listing stays on the shelf until an admin decides. Clearing the store
index the moment they asked would hand them exactly the unilateral delisting
this state exists to prevent — and it makes the decline path free, because
nothing was undone: no key to restore, no version to re-promote.
That forced a distinction the code did not have. `is_published` (exactly
`published`) is now separate from `is_listed` (`published` or
`withdrawal_requested`), and three of the four existing `is_published` call
sites actually meant "live in the store" and moved. A test asserts the two
predicates disagree on exactly one state, so if they ever coincide again the
guarantee has silently broken.
- **One endpoint, two acts.** `DELETE /agents/{id}/listing` branches on state:
a request when the listing is live, immediate when it is not. The author's
intent is identical either way, and making them pick the right verb for their
listing's state is asking them to know the state machine.
- **`POST /admin/agents/{id}/withdrawal`** takes `grant`/`decline`. Deliberately
not folded into `/review`, where "approve" already means "publish" — one
endpoint with four decision values makes an accidental unpublication a
one-character mistake.
- **Requests land in the existing review queue** and the nav badge counts them
(§5.1). A second queue is one an admin has to remember exists.
- **Delete is refused unless the listing is `private` or absent** (§5.2), with a
message naming the way out. `taken_down` is covered deliberately: an author
must not delete their way out of a takedown record.
Two things found while building it:
`AUTHOR_TARGET_STATES` was **dead** — declared and never read, so the mechanism
§5.1 names ("`published → private` leaves AUTHOR_TARGET_STATES") enforced
nothing. It is load-bearing now via `assert_author_target`, because the
transition table alone cannot say "this edge is legal but only for an admin",
and `withdrawal_requested → private` is exactly that.
The `/assistants/{id}` delete path soft-deletes documents and removes sync
policies **before** the record delete, so a refusal discovered at the record
write would leave the Agent gutted and still in the store — worse than either
outcome alone. Hence `assert_deletable` at step 0, sharing one rule function
with the delete itself, and a test that the two agree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sting-withdrawal feat(marketplace): withdrawal becomes a request, and delete respects it (PR-4)
Follow-up to the version-snapshots epic (#784, #787, #789, #791), found by checking the seams between PRs that shipped in parallel and never saw each other. **The leak.** `VERSION#` child rows arrived in PR-2 and were never wired into the delete path, which cleans up `REPORT#` rows and the metadata row and nothing else. So deleting an Agent left its entire snapshot history in the table permanently. `reports.py` states the principle outright — child rows live under the Agent's partition "precisely so they never outlive what they concern" — and versions quietly violated it. Storage-only, no correctness impact, and it would never have surfaced on its own: PR-4 refuses to delete anything but a `private` listing, and a private listing's versions carry no store key, so nothing would ever render them. That is exactly what makes it worth catching deliberately rather than waiting for a bill. Immutability is about *rewriting*, not retention — a version may never be changed, and it is meaningless once the Agent it snapshots is gone. Nothing audits it either: §8 flags "versions referenced by an audit record should survive" as a question for a retention policy that does not exist yet, and there is no such reference today. **Also: two tests for a seam neither PR could have covered.** PR-3 (invocation) and PR-4 (withdrawal) were built in parallel and merged independently, so nothing exercised `withdrawal_requested` through `resolve_invocation_agent`. The behavior is already correct — a pending withdrawal keeps `publishedVersion`, so everyone but the owner still runs the approved snapshot, which is right because a request is not a removal — but it was untested, and it is precisely the kind of thing that regresses silently. Backend 5508 passed / 3 skipped (5504 on develop before this, +4 new). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nt-version-rows fix(agents): delete a deleted Agent's version snapshots
PR-5 of the agent version-snapshots epic (§6.1). The reviewer's actual
question is "what changed since I approved this?", and until now they could
not see it: a submission arrived in the queue with no reference to what it
replaces, so a typo fix and a full instruction rewrite looked identical and
both got the same careful read.
`GET /admin/agents/{id}/diff` returns the pending version against the
published one — changed fields with before/after values, a unified diff of
the instructions, and a `behaviorChanged` flag. The review queue grows a
collapsed "What changed" control per row: behavior changes badge amber,
presentation changes grey, and the instructions diff renders red/green
beneath.
The asymmetry is the point. A tagline fix should be approvable at a glance;
an instruction rewrite should be impossible to miss.
- **The diff is computed server-side** with `difflib`. A client-side library
would render more prettily, but it means a new SPA dependency (this repo
pins exactly and does not add packages casually) and a second implementation
of "did this change" that can disagree with the field-level answer.
- **Collapsed and fetched on expand.** The queue is a list of decisions;
pre-loading a diff per row would pull every pending agent's full
instructions down to render a control nobody opened. Asserted by test,
because it is a property a template edit can silently lose.
- **`DIFF_FIELD_ORDER` is asserted against the snapshot field lists at import
time.** The worst failure available here is a reviewer told "nothing
changed" about something that did, which is exactly what a field added to
`AgentVersion` and forgotten here would produce.
- **"First submission" is a distinct signal**, never an empty change list.
They are opposite claims: one means "read all of this", the other means
"approve it". A dangling `publishedVersion` falls back to the same
rendering — "there is nothing live to compare against" is true, "the author
rewrote everything" would not be.
- **Absent is not empty.** `bindings: null` (synthesize the legacy KB binding)
differs from `[]` (binds nothing), and a laxer comparison would report no
change while the agent quietly lost its knowledge base.
- **Record metadata is never a change.** `version`/`createdAt`/`createdBy`
differ on every snapshot and would bury the fields that matter.
Backend 5553 passed / 3 skipped. SPA 1691 passed across 153 files.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…view-diff feat(marketplace): show a reviewer what a submission changed (PR-5)
ToolFilter knows three tool classes — registry, gateway, external MCP — and warns on anything else. Context-bound tools are a fourth: they need request scope (session/user/assistant) baked in at construction, so inference_api builds them per invocation and passes them as extra_tools, which BaseAgent appends *after* filtering. The filter was never taught about them, so every enabled one logged "not found in registry or catalog, skipping" — and then worked fine anyway. In prod that is ~2,500 false warnings a day (create_artifact 1,744, analyze_spreadsheet 1,690, list_spreadsheets 1,690 over 48h), which drowns the one signal that branch exists to give: a genuinely stale tool id pinned in a saved session's enabledTools. It also cost real time during an incident triage, where it is the first WARNING on a failing turn and points nowhere. The message was also wrong on its face — the filter never consults the catalog, and both spreadsheet ids are in it (tool_catalog.py). Move the per-family gate ids to apis/shared/tools/injected.py so the route (deciding what to build) and the filter (classifying) read one definition, and classify them as a known class. These are gate keys, not tool names — workspace_files provisions list/read/write — so the set can't be derived from what the factories return. get_statistics gains an injected_tools bucket instead of over-counting unknown_tools. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…injected-tool-warning fix(tools): stop warning that context-bound tools are missing
Pre-existing, found while building the Publishers page against the same pattern. `mutate()` set the error message and then called `reload()`, which clears the banner on entry — so the message was wiped before it could render. The visible effect: an admin deleting a category that still has listings in it got a silent no-op. The backend returns a 409 whose message explains the refusal and suggests disabling instead, and none of it ever reached the screen. The reasonable conclusion is that the button is broken. Fixed by capturing the message, reloading, then setting it — with the ordering trap named in a comment, because the obvious order is the wrong one and the next copy of this pattern will reach for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR-6 of the agent version-snapshots epic (§6.3), and the last phase. Purely a UI gap over a finished backend: full publisher CRUD plus the eligibility allowlist has been live at `/admin/agents/publishers` since the marketplace shipped, with no way to reach it — so a profile like "Registrar" or "Communications & Marketing" could only be created by calling the API directly. Adds the page, its route under the `admin.marketplace` scope, a Publishers nav entry, and the SPA service methods (create / update / delete / eligibility). No backend change. The page's job is to make three otherwise-surprising rules legible: - **⚠️ Attribution is a name, not a permission.** Publisher never grants access to anything, and `ownerName` remains who actually owns the agent and whose skills resolve when it runs. Stated at the top and again next to `verified`, which renders as a check mark and is the field most likely to be mistaken for a grant. - **The id is fixed at creation.** Listings store it, so renaming a publisher would strand every attribution pointing at it. The row shows the id and says so — the same rule categories follow, surfaced the same way. - **Disable, don't delete.** Deleting is refused (409) while listings are attributed to the publisher; disabling drops it from the submit picker while existing attributions keep rendering, which is nearly always what was meant. `department` is the default kind because it is what an admin adding a publisher almost always wants; individual profiles are auto-created from an author's display name on first submission, which the empty state says rather than leaving "no publishers yet" reading as broken. SPA 1693 passed across 153 files. Backend unchanged at 5508. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…blishers-page feat(marketplace): admin page for publisher profiles (PR-6)
/chat/api-converse built its User with a hardcoded `roles=["user"]`
placeholder. No AppRole maps the JWT role `user`, so permission
resolution matched nothing, fell back to the `default` role — which
grants no models in prod — and every request 403'd with "Access denied
to model: <id>" regardless of the caller's actual grants.
An API key record stores only key_id/user_id/name, never roles, so the
owner's roles are now read back from the Users table per request: the
same record the cookie-session path enriches from, holding the IdP roles
parsed from the Entra ID token at BFF callback. Email and name come from
the profile too, so quota tier and cost attribution stop being charged
against a synthetic `{user_id}@api-key` identity.
Fails closed — a key whose owner has no profile row is refused rather
than silently degraded to `default`.
Also fixes a cache collision this exposed. `resolve_user_permissions`
derives its result purely from `user.roles` but cached under
`user:{user_id}` alone, so the API-key and cookie paths — same subject,
different roles — shared one entry: whichever resolved first served the
other for the whole 5-minute TTL. That made the bug intermittent and let
an API-key request strip a live SPA session's grants. The key is now
`user:{user_id}:{roles_fingerprint}`, and invalidate_user prefix-deletes
every role-set entry.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
End-to-end testing of the version-snapshots epic (#784–#795) against dev surfaced five reported issues. Four were real; the fifth turned out to be deliberate behaviour and is documented rather than changed. **The detail page served the draft, not the snapshot.** `GET /agents/{id}` had no version overlay, so a published Agent whose author kept editing had a store tile rendered from the approved snapshot and a detail page rendered from the unreviewed draft — different name, different summary, and `capabilities` resolved from the draft's bindings, so the page advertised tools the published version did not have. Invocation ran the snapshot regardless, which made it a lie rather than a preview. `resolve_display_agent` now answers this the same way invocation does, with one deliberate difference: editors keep seeing the draft, because the Agent Designer loads its form from this endpoint and serving an editor the snapshot would make their next save revert the owner's draft. **Withdrawal requests could not be granted.** `POST /admin/agents/{id}/withdrawal` had no SPA caller. The request appeared in the review queue indistinguishable from a submission, so "Request changes" 400'd on an illegal transition and "Approve" silently *declined* it by re-publishing. The queue now labels the row, says the listing is still live while the admin decides, and offers Take it down / Keep published against the endpoint built for them. The author-side control said "Unpublish" and promised removal from the store; it is now "Request withdrawal" and says an admin decides first. **The review diff was unreachable.** Every UI route to a resubmission cleared `publishedVersion` first, so `diff_pending_version` always returned `first_submission` — the "what changed since I approved this" comparison never rendered. `published → changes_requested` is the one transition that preserves it, and nothing exposed it; the Listings page now does. **Publisher delete was unguarded.** `publishers.page.ts` documents a 409 refusal while listings are attributed, and its error handling was already written to surface it, but nothing enforced it — deleting an in-use profile silently unattributed every listing naming it, live ones included, with no surface to repair them and no confirmation first. Adds `publisher_in_use` (mirroring `category_in_use`) and a confirmation dialog. Also: `is_on_shelf` names the "in the store right now" question, because `is_listed` answers by state name and a published listing sent back for changes keeps serving — the featured row and browse disagreed about it. And `AdminListingRow.withdrawalRequestedAt` is what lets the queue tell a withdrawal request from a submission at all. **Not changed:** requesting changes on a live listing does not unpublish it. That is deliberate and asserted by test — the approved version keeps serving until one replaces it, and takedown is the operation that pulls something down. One consequence is left open with a note rather than patched: an author can take such a listing private alone, and closing that needs a product decision, since the transition table cannot allow `changes_requested → withdrawal_requested` without opening a route to `published` for something never approved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…version-snapshot-gaps fix(marketplace): close four version-snapshot gaps found in E2E testing
Finishes the version-snapshots epic: the two §8 open items get decisions, and the behavioural gap #799 deliberately left open gets closed now that the product call behind it is made. **An author can no longer pull a live listing alone.** A listing that was published and then sent back for changes keeps serving (`review_listing` does not unpublish), but sits in `changes_requested` — and `withdraw_listing` asked `is_listed`, which answers by state name, so it read that listing as not-live and took it straight to `private`. #799 documented this rather than fixing it, because the transition table could not allow `changes_requested → withdrawal_requested` without also opening `in_review → changes_requested → withdrawal_requested → published`, a route to published for something never approved. `AgentListing.withdrawalFrom` is what makes the edge safe: a declined withdrawal returns to the state it came *from*, so a request that entered from `changes_requested` can only go back there. Approval stays the only door into the store, and `test_approval_is_the_only_door_into_the_store` now asserts that structurally — decline targets must equal the set of states that can enter.⚠️ Moving that decision onto `is_on_shelf` opened a hole the existing suite caught: `withdrawal_requested → private` is a legal edge (it is how an admin *grants* a withdrawal) and `private` is an author target, so a pending request with no published pointer resolved to `private` and the author granted their own withdrawal. `withdraw_listing` now refuses a second request explicitly rather than relying on the table to imply it. **Rollback shipped** (§8, requested): `GET /admin/agents/{id}/versions` + `POST /admin/agents/{id}/rollback`, with a picker on the admin Listings page. Three constraints: only from `published`, or it is a second door past review; a reason is required and lands on the author's card, as a takedown's does; and no version is cut — it is a pointer move over immutable records, so rolling forward is the same operation. Reuses `_publish_version`, inheriting the new-key-first ordering. **Retention decided: deliberately unbounded.** At a few hundred agents with a handful of KB-sized snapshots each the storage is immaterial, and every alternative costs more than it saves — a TTL deletes invisibly and cannot exempt versions an audit record points at, a keep-last-N prune destroys the older half of a listing's approval history. Rollback makes this stronger, not weaker: an old version is now something an admin can put back, so deleting one costs a recovery path. **Also fixed:** the store read logged a full pydantic traceback per legacy row per browse — listings published before PR-2 still carry GSI5 keys on their `METADATA` item, so the query returns Agent rows that cannot be an `AgentVersion`. Skipped by sort key now. That skip exposed a latent misalignment: `browse_all` paired items to responses positionally, so any dropped row slid every later response onto the previous row's sort key; `_project_with_keys` builds the pairing where the drop happens. And the review diff no longer tells a reviewer looking at an in-review row that there is "nothing awaiting review" when the real cause is a pre-snapshot submission. **Not changed:** the resubmit dialog's category default. #799 reported it as resetting to the first option; it does not — `ngOnInit` preselects the listing's category and clears it only when that shelf has closed. Three regression tests pin the behaviour that was previously untested. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ace-rollback-and-withdrawal-origin feat(marketplace): version rollback, and close the withdrawal origin gap
#800 shipped rollback with the property that makes it safe to use — no version is cut, so the snapshot you rolled off is still there and putting it back is the same pointer move. The dialog said so. The Listings page then hid the only control that could do it. `canRollBack` asked `publishedVersion > 1`. That reads "are we serving above the first version?", which gives the same answer as "does a second version exist?" right up until someone rolls back — and the opposite one afterwards. A listing rolled back to v1 with v2–v5 intact looked identical to one that had only ever had v1, so the button vanished in the one state that most needs it. The endpoint accepts the forward move; verified against dev, the UI simply had no way to ask for it. A rollback you cannot undo is a worse rollback. The row now carries `latestVersion`, derived from `max(publishedVersion, submittedVersion)`. Both counters only move *up* when a snapshot is cut and `submittedVersion` survives the pointer moving down, so the max is ≥ 2 exactly when a second version exists. It can understate the true highest — an admin presentation edit bumps only `publishedVersion`, so a later rollback leaves it one short — which is harmless because nothing reads the number itself, only whether it is above one. No extra read: a page of rows must not fetch every agent's history, and anything needing the real list already calls `list_agent_versions`. Copy follows the behaviour rather than the majority case. "Roll back" named the opposite of what the control does on the sequel to every rollback, so the button is "Change version", the dialog is "Publish a different version", and its reason prompt asks why you are changing the published version. The picker was already correct — it offers everything not currently live, in either direction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#800 taught the diff endpoint to tell a reviewer the real cause when a submission predates version snapshots, instead of claiming there was nothing awaiting review. The message never arrived: `ReviewDiffComponent.load` caught with a bare `catch` and replaced every failure with "Could not load what changed. Try again, or review the agent directly." So the reviewer was told to retry, and retrying could never work — the row is old, not broken. Verified against dev: the backend returns the specific text and names the fix ("ask the author to resubmit — that captures one on the way in"), and the UI discarded it at the boundary. The `detail` is now preferred when the server sent one. The generic line stays as the fallback for the case it was written for: a real transport failure, where there is no `detail` and "try again" is exactly the right advice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rollforward-and-diff-detail fix(marketplace): keep rollback reversible, and tell the reviewer why a diff is missing
The badge was already on `secondary` tokens but read as amber. The scale derives every step from `#d64309` by moving lightness alone and holding chroma, which walks the colour out of gamut in both directions: the 50 step clipped to a pale yellow, and 600 clips toward pure red. Only 500 is a literal hex, and it is the value the New Session `+` above is drawn in — so the badge now fills with it and matches, sampled byte-for-byte. White on 500 measures exactly 4.50:1, clearing the AA floor for 10px text with nothing to spare; noted at the call site so a future darkening of the text or lightening of the fill gets re-measured rather than assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Who can open an agent was answered in three unrelated places: a people list and a link in the share dialog, publication controls on the agent list's cards, and nothing tying them together. They are one question asked at three widths, so the dialog now reads as a ladder — People with access → General access (the link) → Marketplace (found without one). Cards go back to being index entries: icon, name, one line, four verbs. The model chip, the tool/skill/memory counts and the whole publication rail came off; they turned a page you scan into a page you read. The preview panel's capability strip goes for the same reason — it restated the form sitting one column to its left, so it could only ever agree or be wrong. Two bugs this pairing would otherwise have introduced: * Publishing from inside the dialog flips visibility to PUBLIC mid-session, while the save path derives PRIVATE/SHARED from the people list. Deriving over a fresh PUBLIC would narrow a published agent out from under its own listing — the store keeps serving the tile while every visitor 404s on open. Visibility is now a signal publication writes to, and the derivation refuses to touch PUBLIC. Both branches are tested. * The old dialog's PUBLIC branch showed only a URL, so the owner of a public agent could not see, let alone revoke, who else held editor. Shares now load for every visibility. Also here: * The dialog moves to `agents/components/`. It needs the listing service and submit dialog from `agents/`, and all three callers already point agents → assistants; leaving it put would have closed a cycle. It fetches its own listing rather than taking one, so every caller gets the marketplace section without threading it through, and the narrower data type drops the `as unknown as` casts all three carried. * The two "Search users" / "Add by email" tabs collapse into one field. Asking someone to classify what they are about to type before typing it bought nothing: names hit the directory, and anything that parses as an address — including a comma-separated run — offers to add outright. Runs are all-or-nothing, because a partial accept reads as "added" while the dropped people never hear about it. * The store icon moves to Persona, beside the emoji. Publishing is owner-only but the icon is presentation, which D13 lets an editor set; folding it into an owner-only dialog would have quietly revoked that. * Dialog rebuilt on the canonical tokens (`add-curated-model-dialog`): sectioned header/body/footer, `bg-gray-900/40` backdrop, and `max-h-[90vh]` with a scrolling body — the old one had no height containment and ran off a short viewport. * Model selection is a stroke, not a `primary-50` fill. Dark mode steps to `primary-300`: measured against the dark card the brand navy is a 1.45:1 stroke and `primary-400` only 2.24:1, both under the 3:1 floor for a non-text indicator (WCAG 1.4.11). The fill had been hiding that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hare-surface-and-card-simplification feat(agents): make the share dialog the one surface for reach
…-hydration fix(api-keys): resolve the key owner's real roles for RBAC
The roles list rendered JWT mappings and tool grants on each card but nothing about admin scopes, so a system admin scanning the page could not tell which roles carry admin power without opening each role's edit form. That cuts against the invariant the feature rests on — granting a scope is a `system_admin`-only act, and one worth being able to watch. Adds an amber badge on the role title and an "Admin Access" cell to the details grid, with labels resolved from the scope registry. `system_admin` is special-cased: it holds every scope implicitly and so carries an empty `grantedAdminScopes`. Rendering that as "no admin access" would be exactly backwards, so it badges as "Full Admin" / "All Areas". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`admin_service` has emitted structured log records on every role mutation since before delegated admin scopes existed. Those are log lines: no retention guarantee, no before/after values, and nothing an admin can read from the console. With one superuser that was tolerable. Now that admin power can be delegated, "which admin granted this, and when?" is a question the platform has to be able to answer. Adds `apis/shared/audit` (record, repository, service), an `audit-log` DynamoDB table with a one-year TTL, a read-only `/admin/audit` API, and an Audit Log console page under Identity & Access. Three decisions worth review: - **A record is a diff, not a snapshot.** `before`/`after` carry only the fields a mutation changed. The diff is taken against a pre-mutation deepcopy, because `update_role` writes onto the fetched role in place — and against the object rather than the request body, because the role form posts every field on every save. - **Four of the eight emission points get no record of their own.** The tool and skill grant helpers, and the write-through path the admin pages use, all build an `AppRoleUpdate` and delegate to `update_role`. Emitting from both would write two rows per mutation. `ROLE_UPDATED` already carries the exact grant-list before/after. - **Refused mutations are recorded.** `app_role.mutation_denied` fires when the write-through guard raises. It is the only record where nothing changed and the only place an attempted escalation is visible. Reads are `system_admin`-only via a non-delegable `admin.audit` scope, and the API exposes no mutating routes — records age out via TTL and no other way. An audit write never fails the mutation it describes, and a missing table name means no sink rather than an error: the table ships in platform.yml while this code ships in backend.yml, and backend deploys don't run CDK. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tribute-chain bypass Two independent defects that chained into attacker-authored OS command execution inside another principal's chat session. 1. Cross-owner write on the admin per-object skill route. GET /admin/skills/ narrows to owner_id == "system", but every per-object route read the row by id with no predicate, so an actor holding only admin.skills could read and rewrite a private, user-owned skill's instructions — instruction-trusted content that steers its owner's agent — while the owner was 403 on the same route. SkillCatalogService now exposes get_catalog_skill() / require_catalog_skill(); the predicate is applied in the service for update_skill, delete_skill, list_resources and the role-grant methods, and at the route layer for GET and the reference-file routes. Non-catalog rows now return 404 with the same message template as a nonexistent id, so the surface no longer confirms that another user's private skill exists (the old "is user-authored" role-grant error was itself an existence oracle). 2. AST allowlist bypass in the code-execution sandbox policy. visit_Attribute checked only for dunders, so an allowlisted module that itself imports a host module re-exported it: pd.io.common.os.popen(...) reached the full os module with no import statement in the submitted source. Attribute nodes are now checked against the same denylist as bare names, ImportFrom members are checked too (from pandas.io.common import os as o bound the real module under an innocuous name), the missing host modules are covered (builtins, gc, runpy, posix, nt, codeop, pdb, bdb, timeit, webbrowser), and the adjacent deserialization sinks are closed — a pickle payload can arrive as a bytes literal through io.BytesIO, so read_pickle/to_pickle/read_hdf/to_hdf/load are refused, as are process-spawn entry points as a second net. Documented consequence of the attribute rule: a column colliding with a denied name must use df['open'], and a lazily-loaded submodule must be imported directly (from scipy.signal import butter). Both were already true for the bare-name form. Verification: the disclosed payload run through both policy versions — pre-fix ACCEPTED, post-fix REJECTED (forbidden attribute: popen). Full backend suite in a clean worktree: 32 failed / 6776 passed at develop, 32 failed / 6804 passed with this change (same pre-existing artifact_render and crawl_repository failures, +28 new tests, zero new failures). No new ruff or mypy findings. Not addressed here: the sandbox policy remains a denylist over a large library surface, and instruction-trusted content still reaches other principals by design through invoke-through on a shared Agent. Capability reduction inside the execution environment is the durable control and is separate work.
…-cross-owner-write-and-ast-attribute-chain-bypass fix(security): scope admin skills routes to the catalog; close AST at…
Fixes a High-severity OIDC login CSRF / session fixation hole in the BFF
auth flow (finding f-8c4f312a).
`GET /auth/login` minted a `state`, stored it server-side, and redirected
to the Cognito Hosted UI without issuing any browser-side material — no
state cookie, no PKCE, no nonce. `state` is a public value: it travels in
a 302 Location and anyone can mint one anonymously. `GET /auth/callback`
nonetheless treated "this state exists in the store" as proof the request
continued a login *this* browser started, so it accepted any (code, state)
pair from any browser.
Exploit: an attacker mints a state anonymously, authenticates at the IdP
themselves to get a real authorization code, then lures a victim to
/auth/callback?code=<theirs>&state=<captured>. The victim's browser is
silently issued a live session for the attacker's account — reported
against a `system_admin` identity — and everything the victim then does
(conversations, uploads, memory entries, third-party connector consent)
lands inside an account the attacker still holds credentials to.
Browser binding is the fix:
- /auth/login mints a 32-byte secret, returns it in a new
`__Host-bff_oauth_state` cookie (HttpOnly, Secure, Path=/, no Domain,
SameSite=lax), and commits only its SHA-256 digest to the state row.
- /auth/callback recomputes the digest from the cookie and refuses the
exchange unless it matches, via `secrets.compare_digest`. The
attacker's cookie is in the attacker's jar, so the victim fails closed
before the code ever reaches the token endpoint.
Three deliberate choices:
- The cookie is checked *before* the state-store lookup, so a probe from
a crafted link can't burn a user's in-flight state.
- `SameSite=lax` is required, not a compromise: the IdP returns the user
via a top-level cross-site GET, which `strict` would withhold.
- A state row carrying no digest fails closed. Those exist only
mid-deploy; honouring them would keep the hole open for the whole
rollout window, which is exactly when an attacker holding a pre-minted
state would strike. Cost is one retry for logins spanning the deploy.
Also adds PKCE (S256) and OIDC nonce verification end-to-end: the verifier
and nonce live in the state row and never reach the browser; the verifier
is sent to /oauth2/token and the nonce is compared against the ID-token
claim. Both are defense in depth (code interception, token substitution) —
PKCE alone would NOT have closed this finding, since the attacker drives
the BFF-minted authorize URL and the stored verifier matches their code.
No Origin / Sec-Fetch-Site check was added, despite the report suggesting
one: a legitimate arrival at /auth/callback *is* a top-level cross-site
GET carrying `Sec-Fetch-Site: cross-site` and no `Origin`, so it is
indistinguishable from the attacker's link. Rejecting on those headers
would break every login and stop nothing. A test pins that those headers
are still accepted and that binding carries the rejection.
Verification: the new tests were confirmed to catch the vulnerability, not
just the implementation — neutering the three enforcement conditions fails
12 of them, including the full-chain test. Full backend suite is 6840
passed / 35 failed, with the 35 byte-identical to a baseline captured with
this work stashed (pre-existing, in web_sources and artifact_render).
Tests:
- test_login_csrf_regression.py (new): replays the reported chain against
the real /auth/login with no hand-seeded state, covers the free-retry
observation and forged cookies, plus a positive control that the
originating browser still completes login.
- test_callback.py: browser-binding, PKCE and nonce enforcement; seeded
state rows now carry the binding digest.
- test_login.py: cookie attributes, digest-only storage, per-request
uniqueness, secret absent from the redirect URL, S256 challenge
derivation, nonce/state agreement.
Incidental: annotate `_SAMESITE` as `Literal["lax"]`, clearing five
pre-existing mypy arg-type errors on Starlette's `samesite` parameter.
…in-csrf-state-binding-pkce-nonce fix(auth): bind BFF OAuth state to the requesting browser
An authenticated, zero-privilege user could plant JavaScript that later executed on the SPA's own origin inside a system_admin's authenticated browser session (finding f-317ac252). Two control failures chained: 1. WRITE — the skill-resource upload routes persisted the client-supplied multipart Content-Type verbatim with no allowlist, and permitted an .html filename, so an ordinary user could store bytes labelled text/html. 2. READ — the read routes reflected that stored type as the response media type with `Content-Disposition: inline`, and the CloudFront /api/* behavior carried no response-headers policy, so responses shipped without nosniff and without a CSP (GET / had both). Because app-api is served from the same origin as the Angular SPA, the uploaded file parsed as a top-level HTML document and its inline <script> ran with the viewer's session — reading the non-httpOnly CSRF cookie and making arbitrary state-changing admin calls drivable as system_admin. Three independent layers, so no single one is load-bearing: * Upload allowlist. New `apis/shared/skills/resource_types.py` derives the stored media type from the filename extension against an allowlist whose every value is inert, and ignores the client-supplied Content-Type entirely — the same posture the agent-icon route already takes. .html/.htm/.xhtml/.xml/.svg are refused; source-code extensions are allowed but collapse to text/plain, matching the existing spec-D5 rule that script resources are stored inert. Applied in `SkillCatalogService.add_resource`, which both the admin catalog tier and the user-authored "My Skills" tier funnel through. * Read hardening. Both read routes now re-derive the served type from the filename instead of reflecting the row, and respond with `attachment` + `X-Content-Type-Options: nosniff` + `default-src 'none'; frame-ancestors 'none'; sandbox`. Re-deriving at serve time is what neutralizes rows written before the allowlist existed, so no data migration is needed. `attachment` is safe because both SPA callers fetch these over XHR as text for an in-app viewer; nothing navigates to the URL. * Edge backstop. The CloudFront /api/* behavior gets its own ResponseHeadersPolicy (nosniff, `default-src 'none'; frame-ancestors 'none'`, X-Frame-Options DENY, HSTS, no-referrer) so a future route cannot regress the whole origin. `sandbox` is deliberately omitted at the edge: `default-src 'none'` already blocks all script in a document, and an opaque-origin directive across every API response would risk breaking attachment downloads and OAuth navigations for no added protection. Also mirrors the allowlist client-side (`shared/skills/skill-resource-types.ts`) so both skill forms filter the file picker and refuse a bad file with a specific message instead of a 400 mid-upload. UX only — the server is the control. Note: the cross-owner read half of the finding (the admin route resolving a user-authored skill) was already fixed by the `require_catalog_skill` predicate in 632e1ac; the two failures above were still live, and either alone reproduces the escalation for a catalog skill's resources. Tests - backend/tests/apis/app_api/skills/test_skill_resource_mime.py — 36 tests reproducing the report's upload and read steps with the verbatim payload, every scriptable extension, and a planted legacy text/html row. 17 of the 36 fail without the source changes. - infrastructure/test/api-security-headers.test.ts — asserts the /api/* policy exists and is attached to that behavior. Both tests fail without the construct change. - frontend .../skill-resource-types.spec.ts — pins the client mirror against drifting open on the dangerous extensions. Verified: 165 backend tests pass (skills + architecture), 100 CDK tests pass across the 8 CloudFront/SPA suites, frontend tsc clean, production build succeeds, 52 frontend tests pass.
…ce-mime-allowlist-and-inline-xss fix(skills): close privilege-escalating stored XSS in skill resources
Strands' stock ModelRetryStrategy retries ModelThrottledException and nothing else, and BedrockModel maps exactly one error code to it (ThrottlingException). Every other Bedrock fault re-raises as a raw botocore ClientError, so the configured four-attempt backoff never ran for them. Prod session 5f34d2b0 (2026-08-31): a ConverseStream carrying two PDFs failed with ServiceUnavailableException after 95.6s, was never retried, billed 56,440 uncached input tokens, and returned zero output. Meanwhile ThrottlingException — the one error we did cover — has not occurred once in prod in the last eight days. BedrockTransientRetryStrategy subclasses the SDK strategy and widens only is_retryable, so backoff policy and attempt budget are inherited unchanged. It never narrows the stock behavior. Mid-stream failures are deliberately excluded: EventStreamError is raised while iterating the response stream, after chunks may already have reached the client, and restarting there would replay visible output. A plain ClientError from the converse_stream call itself means the request was rejected before the stream opened, so a retry is invisible. RETRY_TRANSIENT_SERVICE_ERRORS=false restores stock behavior without a deploy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Inline document bytes are one-shot. _strip_document_bytes removes them from restored history because Bedrock rejects two document blocks sharing a sanitized name — correct when a turn succeeds, but when a turn dies before the model reads them they are gone with no way back. In prod session 5f34d2b0 that turned one transient 503 into a dead end: the two PDFs died with the failed request, so the assistant asked the CIO to upload them again, and the re-upload turn failed the same way. Adds a write-ahead marker on the session row. The invocations route records the turn's upload IDs before the model call; StreamCoordinator clears it the moment the turn produces assistant content (including the max_tokens branch, which consumed the documents before hitting the output cap); the next turn pops it and re-sends. Written ahead rather than from an error handler so it survives every way a turn can die — including a dropped stream that no error arm ever sees. Bounded deliberately: the pop clears as it reads, so recovery can only influence the single turn following a failure, and a marker older than an hour is discarded. The user's own attachments always win — recovered IDs are dropped entirely rather than merged, since the resolver caps a turn at five files and merging could push out a file they just attached. A recovery note tells the model the files were re-sent and that the user did not attach them again, so it stops asking for uploads the system already has. It rides the existing original_message displayText split, so the user never sees it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two gaps left open by the 5f34d2b0 post-mortem, both about the user having no idea what the system is doing. A retry looked like a hang. Strands emits EventLoopThrottleEvent on every retried model call and nothing consumed it, so a retry was silence. That got worse the moment we widened the retryable set, since a 503 now buys several extra seconds of it. The processor now emits a `model_retry` SSE event and the SPA swaps the loading indicator's cycling phrases for a fixed amber notice, cleared on message_start/done. Honest about what this does not do: Strands sleeps the backoff inside its hook and yields the event afterwards, so the notice lands as the next attempt begins rather than when the wait starts, and it cannot cover the failing model call itself — 95 seconds in the incident — which is indistinguishable from a slow healthy one. That needs a separate heartbeat. A 503 read as a generic error. Two classifiers both keyed on "throttl" and neither recognized service-unavailable, so the CIO got "I ran into a problem with the AI model" for an outage that had nothing to do with the request. Adds a shared is_service_unavailable_error predicate — shared precisely because the two classifiers previously disagreed by omission — and copy that says the fault is on the provider's side and that we already retried. Kept narrow: "unavailable" alone would swallow unrelated copy like "the model or feature you're trying to use isn't available". The override sits outside the per-code branches because a 503 surfaces under MODEL_ERROR, STREAM_ERROR or AGENT_ERROR depending on where it is caught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`_get_session_by_gsi` returned None both for "no such session" and for
"exists, but owned by someone else". Callers could not tell those apart,
so `ensure_session_metadata_exists` read the second case as the first and
created a SECOND metadata row on the same session id under the requester.
Its `attribute_not_exists(PK)` guard cannot catch this — the new row has a
different PK (`USER#{requester}`), so the conditional put succeeds.
Session ids travel in shareable `/s/{sessionId}` URLs. Opening someone
else's link 404s on the metadata read, but the SPA then treats the session
as new and lets the user send, which is what forked it. Observed in prod
on 2026-08-31.
NOT a confidentiality bug: conversation content lives in AgentCore Memory
keyed by actor id, so the second user only ever saw an empty thread. The
damage was the duplicate row, the spend attached to it, and — because both
rows share GSI_PK/GSI_SK and DynamoDB returns them in an unspecified order
— the original owner's session resolving non-deterministically afterwards,
which is why that turn logged "update_session_activity: session missing
and could not be created" against a session that plainly existed.
Three changes:
- `session_owned_by_other_user` makes the distinction explicit, fail-open.
- `ensure_session_metadata_exists` refuses to create over another owner,
and the invocations route rejects the turn with 404 (not 403 — it says
nothing about whether the session exists, matching the metadata GET).
- Both GSI lookups now scan every returned row for the caller's own,
instead of reading items[0]. Forked rows already exist in prod, so the
read path has to stay deterministic over them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…transient-retry-and-attachment-recovery fix: survive a transient Bedrock outage instead of dead-ending the turn
…ross-user-fork fix(sessions): stop a session id being forked across two users
Closes the last gap from the 5f34d2b0 post-mortem. The model_retry event covers the pauses BETWEEN attempts; it cannot cover the failing call itself, which is the longest silence and the one that actually loses sessions. Both dead turns in that incident ran ~95 seconds with no output and the user abandoned both — the second while the request was, as far as the telemetry shows, still in flight, with no Bedrock outcome ever recorded for it. The parser now stamps the arrival time of every event, and the loading indicator says "Still working…" after 30s of silence and "Still working — this is taking longer than usual." after 90s. Thresholds sit past a normal first token (~5-7s) and past most tool calls, so a healthy turn rarely trips them. A known retry outranks the stall notice: one is a fact, the other an inference from elapsed time. Liveness is stamped on EVERY event, including ones the stream-state gate then drops — the question is whether the connection is alive, not whether the payload was useful — but only after the stale-stream guard, so a superseded stream cannot keep its replacement looking alive. DELIBERATELY CLIENT-SIDE. A server-sent heartbeat would have to race the agent stream against a timer, which means either running __anext__ from a fresh task per element (breaks anyio cancel scopes inside the MCP clients — entering and exiting a scope from different tasks is exactly what anyio forbids) or pumping the stream through a queue in a side task (a new task boundary through the cancellation path that owns lease release and interrupted-turn persistence, which #653/#863/#874 have each already had to repair). The SPA holds the one fact that matters — when the last byte arrived — and a dropped connection surfaces through fetch-event-source as an error rather than as silence, so silence on an open stream really does mean the server has not sent anything yet. The ticker runs only while a response is pending; an always-on interval would wake every open conversation to answer a question nobody is asking. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
feat(chat): tell the user a quiet response is still working
A completed turn could leave an interrupted-turn marker behind, and the NEXT
turn then fed the model a false account of the conversation.
HOW THE MARKER GETS THERE. The client's Stop writes `lastTurnInterrupted`
immediately (app-api, source=client_signal), but the server only observes the
armed cancel on the lease heartbeat — and that loop sleeps
LEASE_HEARTBEAT_SECONDS (10s) BEFORE its first check:
while True:
await asyncio.sleep(LEASE_HEARTBEAT_SECONDS)
cancel_requested = await renew_session_lease(lease)
So a turn that finishes inside that window races the first tick and wins. The
stream completes normally, `session_manager.cancelled` is never flipped, the
cooperative-stop arm never runs, and the marker survives describing a turn
that was never cut short.
WHY THAT MATTERS. The next turn pops the marker and prepends
`_build_interruption_note("user_stopped")`, which tells the model that "the
user deliberately stopped your previous response before it finished (the last
assistant message above is the partial that was delivered)" and to "not resume
or repeat it". Every clause is false when the answer was complete, and the
note demonstrably steers the following answer. A reload also shows the
"response interrupted" affordance against a complete message.
Verified in dev 2026-09-02: Stop at 2.6s on a 10.4s turn; full answer
persisted; the follow-up turn logged "Cleared interrupted_turn ...
(reason=user_stopped)", i.e. the note fired against a complete reply.
THE FIX is the same reconciliation already used for pending attachments: a
turn that reaches the end of the success path produced a complete answer, so
it was not interrupted, whatever the client signalled. The genuine
interruption arms sit in `except` blocks and re-set the marker after
persisting their partial, so a real interruption is untouched.
Deliberately NOT fixed by retuning the heartbeat: a turn shorter than one tick
is unstoppable however the ticks are spaced, and check-then-sleep just moves
the first check to t=0, before any Stop can have been pressed. The marker has
to be reconciled against what actually happened.
Pre-existing bug, unrelated to the 5f34d2b0 outage work — found while
validating those fixes against dev.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The stack had 13 CloudWatch alarms and none of them notified anybody. Three
constructs carried a comment saying so. Two of those alarms were worse than
silent: they watched metric names that exist in no CloudWatch namespace, so they
sat in INSUFFICIENT_DATA from the day they were created, which an operator reads
as healthy.
Every alarm now publishes to one SNS topic. 77 alarms, zero unrouted.
Routing is structural, not conventional. AlarmFactory attaches AlarmActions and
OKActions as a consequence of being used at all, so an unrouted alarm requires
deliberately bypassing it, and a source-level test fails the build if anyone
calls new cloudwatch.Alarm() directly. The previous gap was not carelessness —
the broken form was the shorter one.
Verified against the live account rather than documentation:
- The AgentCore alarms used namespace `bedrock-agentcore` with InvocationCount /
InvocationErrors / InvocationLatency. That namespace is real but holds only the
OpenTelemetry/Strands application metrics; those three names exist nowhere.
Corrected to AWS/Bedrock-AgentCore with the verified Resource + Operation +
Name dimensions, split so SystemErrors (AWS's fault) is separate from
UserErrors (ours), plus a new throttle alarm.
- The latency threshold of 30s sat BELOW the observed maximum. Measured over 14
days, turns average 3.0-4.5s with daily peaks to 24.4s, because the chat path
is SSE and the runtime does not finish a request until the stream closes.
Floors now default to 120s. AgentCore Latency is in milliseconds while ALB
TargetResponseTime is in seconds — a 1000x trap in either direction, so both
units were confirmed with get-metric-statistics.
- DynamoDB has never throttled: ReadThrottleEvents, WriteThrottleEvents and
SystemErrors all had zero metric streams, since every table is on-demand.
Account-level UserErrors had live data with nothing watching it. That traded 78
alarms on signals that have never fired for 27 that include one firing today.
- No Cognito failure alarm and no Browser alarm. AWS/Cognito on the ESSENTIALS
feature plan publishes only success metrics, and Browser has zero streams.
Alarming on either would recreate the permanently-green alarm this change
exists to remove. Both omissions are asserted by tests.
New coverage: ALB (6), ECS service (3), DynamoDB (27), Lambda (21), AI path (9),
AgentCore Runtime (4). Plus a {prefix}-platform-health dashboard ordered by
triage rather than service inventory, which links to the two existing dashboards
instead of restating them — keeping the stack at exactly 3, the CloudWatch free
ceiling.
Configuration is 18 single scalars with cost-conscious defaults. No
config.production branching: this repo is forked by many institutions, so a fork
with one environment should not reason about a production boolean and a fork with
three should not be limited to two. Per-environment values live in the forker's
deployment config. Enforced by test. The clearest case is X-Ray sampling, which
was fixedRate 1.0 for any fork that never set production — a recorded trace for
every agent invocation at $5/million. Now 1% by default.
Log retention becomes one configured value across all 15 groups, replacing 14
hardcoded literals (13 ONE_WEEK plus one ONE_MONTH that differed silently). Two
gaps needed more than per-site edits: the AgentCore Runtime's group is created by
the service rather than CloudFormation, so an AwsCustomResource calls
PutRetentionPolicy on it; and CDK creates groups for its own machinery that
default to 731 days and are declared nowhere here, so a LogRetentionAspect
rewrites every group in the tree. The second was found by diffing a real cdk
synth — unit tests missed it because a bare cdk.App lacks the cdk.json feature
flags that materialise those groups.
Resource budget: 382 of CloudFormation's 500-resource limit, up from 308. This is
a deliberate single-stack architecture with nowhere to spill, so the DynamoDB
allocation was decided by measurement rather than by covering every documented
metric. A guard test fails above 460.
Subscriptions are deliberately not infrastructure-as-code. Several teams need to
hear about failures and their membership changes far more often than the
infrastructure does; requiring a PR and a deploy to add one address is how a
notification list goes stale and stops being trusted. A test asserts zero
subscription resources exist so the decision cannot be quietly reversed. The
required post-deploy step is documented in step-05-verify.
Verification: real cdk synth produces 382 resources, 77 alarms with 0 empty
AlarmActions, 15 log groups all at the configured retention. Full suite 781 tests
across 40 suites, sharded 4 ways. Guards proven non-vacuous by planting a file
violating all three rules and confirming three tests fail.
Comment density in the new constructs ran 33-56%, and the platform.yml env block had 22 lines of prose above 18 variables. Most of it explained things a reader already knows or restated decisions that belong in the steering doc. Removed ~980 net comment lines. What stayed is the set of facts someone would otherwise get wrong: CloudWatch cannot publish to an alias/aws/sns-encrypted topic and needs GenerateDataKey* rather than just Decrypt; CloudWatch caps math-expression alarms at 10 metrics while CDK defaults to 14 operations; AgentCore Latency is milliseconds where ALB TargetResponseTime is seconds; Code Interpreter publishes Resource as a bare id where Memory and Gateway use ARNs; UnHealthyHostCount and RunningTaskCount stop being published rather than reporting zero; CDK's own provider log groups default to 731 days. Constructs now sit at 11-27% and tests at 4-17%. No behaviour change. Full suite 781 tests across 40 suites, sharded 4 ways.
…upted-marker fix(chat): don't tell the model a completed answer was cut short
Add production observability baseline with routed alarms
… runtime scope Admin "Discover from server" (POST /admin/tools/discover) signs its request with the app-api task role, not the gateway role the form's credential picker names — the gateway only signs at runtime, after the target is registered. That role had lambda:AddPermission/RemovePermission/GetFunctionUrlConfig but never lambda:InvokeFunctionUrl, so discovery returned 403 for every AuthType=AWS_IAM MCP server in every environment and admins had to type each tool name by hand. Separately, the runtime role's ExternalMCPLambdaAccess was scoped to `<prefix>-mcp-*` only, which never matches an MCP server deployed from its own repo as `mcp-<server>-<env>`. Harmless while such a server is a Gateway target (the gateway role gets a per-target resource-policy grant), but a 403 the moment one is configured as a direct external MCP tool. Both grants now cover both naming conventions. Adds a guard test asserting both statements carry InvokeFunctionUrl on both resource patterns; it fails against the pre-fix scopes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…mcp-discovery-invoke fix(infra): grant MCP Lambda function URL invoke to app-api and widen runtime scope
Minor release on reliability, security and observability.
- Production observability baseline: 77 alarms, zero unrouted, all publishing to one KMS-encrypted SNS topic, plus a platform-health dashboard and an 18-scalar observability config section. The stack previously had 13 alarms, none routed, two of them watching metric names that exist in no namespace.
- The 5f34d2b0 outage post-mortem: Bedrock's transient service faults are retried, a retry (`model_retry` SSE event) and a long silence ("Still working…") are both visible to the user, and attachments a failed turn never delivered are re-sent on the next one.
- Four security findings closed: High-severity OIDC login CSRF in the BFF auth flow (browser binding + PKCE + nonce), privilege-escalating stored XSS in skill resources (upload allowlist + serve-time re-derivation + CloudFront /api/* header policy), cross-owner write on the admin skill routes, and an AST attribute-chain bypass in the code-execution sandbox.
- Managed Knowledge Base migration hardened against its first real runs in dev — eleven defects, most from one root cause: the two engines were never made exclusive. Still off by default.
- A completed answer no longer tells the model it was cut short; a session id can no longer be forked across two users; admin MCP tool discovery no longer 403s on IAM-authenticated servers.
No DynamoDB index operations in this release. Requires a CDK deploy plus one manual step: subscribe to the new {prefix}-alarms topic.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Comment on lines
+2
to
+14
| import { loadConfig, AppConfig, | ||
| OBSERVABILITY_DEFAULT_AGENTCORE_ERROR_THRESHOLD, | ||
| OBSERVABILITY_DEFAULT_ALB_TARGET_5XX_THRESHOLD, | ||
| OBSERVABILITY_DEFAULT_DYNAMO_THROTTLE_THRESHOLD, | ||
| OBSERVABILITY_DEFAULT_ECS_CPU_PERCENT, | ||
| OBSERVABILITY_DEFAULT_ECS_MEMORY_PERCENT, | ||
| OBSERVABILITY_DEFAULT_LAMBDA_DURATION_PERCENT_OF_TIMEOUT, | ||
| OBSERVABILITY_DEFAULT_LAMBDA_ERROR_THRESHOLD, | ||
| OBSERVABILITY_DEFAULT_LOG_RETENTION_DAYS, | ||
| OBSERVABILITY_DEFAULT_P99_LATENCY_MS, | ||
| OBSERVABILITY_DEFAULT_XRAY_SAMPLING_RATE, | ||
| OBSERVABILITY_DEFAULT_XRAY_SAMPLING_RESERVOIR, | ||
| } from '../lib/config'; |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| instructions="benign", | ||
| ) | ||
|
|
||
| assert client.delete(f"/skills/{skill.skill_id}").status_code == 404 |
| ) | ||
|
|
||
| assert client.delete(f"/skills/{skill.skill_id}").status_code == 404 | ||
| assert client.delete(f"/skills/{skill.skill_id}?hard=true").status_code == 404 |
Comment on lines
+280
to
+283
| assert ( | ||
| client.delete(f"/skills/{skill.skill_id}/resources/notes.md").status_code | ||
| == 404 | ||
| ) |
| from __future__ import annotations | ||
|
|
||
| import asyncio | ||
| import os |
| """ | ||
|
|
||
| def _lambda_timeout_minutes(self) -> int: | ||
| import re |
CodeQL only analyzes `main`, so a release PR is the first scan these commits have ever had. Four alerts landed; three are real. - py/log-injection (medium), inference_api/chat/routes.py. The cross-user-fork rejection logs `input_data.session_id`, which comes straight off the request body, so a crafted id could forge log lines. The sink is close to unreachable in practice — the branch only runs when the id already exists and is owned by someone else, so it is a real generated id — but the file already has `_sanitize_log` for exactly this and the call now goes through it. - py/unused-global-variable (note), kb_backend/provisioning.py. `KB_PENDING_STATUSES` was defined and documented but never read: the ACTIVE wait branches on KB_ACTIVE_STATUS and KB_FAILED_STATUSES and treats everything else as "keep waiting". Removed rather than wired in, since consulting it would change behaviour for an unknown status. - py/multiple-definition (warning), same file. `last_status = "UNKNOWN"` before the loop is a dead store; the loop body assigns it from the describe call before any read. The fourth (py/clear-text-logging-sensitive-data, high, bff/routes.py) is a false positive and is left alone — the SARIF flow runs StringLiteral -> OAUTH_STATE_COOKIE_NAME -> import -> log, so the "sensitive data" is the constant "__Host-bff_oauth_state" itself. It is a fixed cookie name that ships in every Set-Cookie header; no runtime value, user input or secret is on the path. The actual secret in that function, `binding_secret`, is never logged — only its SHA-256 digest. Tests: 400 passed across managed-kb, kb-migration worker, inference_api and the BFF auth suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Minor release on reliability, security and observability. 14 PRs (#889–#911), 135 files.
Pre-merge check: DynamoDB GSI limit — PASSED
infrastructure/gsi-inventory.jsonis unchanged. No table gains or loses an index in this release, so the one-GSI-operation-per-UpdateTablelimit is not in play and no split is required.What's in it
Production observability baseline (#910). The stack had 13 CloudWatch alarms and none of them notified anybody — three constructs carried a comment saying so. Two were worse than silent: they watched metric names that exist in no CloudWatch namespace, so they had sat in
INSUFFICIENT_DATAsince creation, which reads as healthy. Now 77 alarms, zero unrouted, all publishing to one KMS-encrypted SNS topic, plus a{prefix}-platform-healthdashboard and an 18-scalarobservabilityconfig section. Routing is structural:AlarmFactoryattaches actions as a consequence of being used, and a source-level test fails the build on a directnew cloudwatch.Alarm().Outage post-mortem, session
5f34d2b0(#905, #907). AConverseStreamcarrying two PDFs failed withServiceUnavailableExceptionafter 95.6s, was never retried, billed 56,440 uncached input tokens and returned nothing — twice, because the attachments died with the first request. Bedrock's transient faults are now retried (pre-stream only, so a retry can never replay visible output); a retry (model_retrySSE event) and a long silence ("Still working…") are both visible to the user; and attachments a dead turn never delivered are re-sent via a write-ahead marker.Four security findings closed (#902–#904). High-severity OIDC login CSRF / session fixation in the BFF auth flow — closed with browser binding plus PKCE and nonce. Privilege-escalating stored XSS in skill resources — closed at three independent layers. Cross-owner write on the admin skill routes, and an AST attribute-chain bypass in the code-execution sandbox; those two chained into attacker-authored OS command execution inside another principal's chat session.
Managed Knowledge Base hardening (#889, #898–#901). Eleven defects from its first real runs in dev, most of them one root cause: the two engines were never made exclusive. Documents indexed twice, a
statusfield with two writers racing to decide "ready", and deletions never reaching the managed engine. Still off by default — everyCDK_MANAGED_KB_*flag remains OFF and these fixes are inert unless armed.Correctness (#906, #909, #911). A completed answer could tell the model it was cut short; a session id could be forked across two users via a shared
/s/{id}link; admin MCP tool discovery 403'd on every IAM-authenticated server in every environment.Version
1.16.0→1.17.0(minor — new capabilities, no breaking API changes).scripts/common/sync-version.sh --checkpasses across all 9 manifests and lockfiles.Deployment
platform.yml→backend.yml→frontend-deploy.yml, all three required.One manual step CDK cannot do: subscribe the team to the new
{prefix}-alarmsSNS topic (step-05-verify §6). Subscriptions are deliberately not IaC. Until then the alarms are still unrouted.Takes effect with no flag: log retention 7 → 30 days; X-Ray sampling 100% → 1%; Bedrock transient faults retried (
RETRY_TRANSIENT_SERVICE_ERRORS=falseto opt out, no deploy needed); skill-resource uploads reject scriptable extensions and reads serve asattachment; logins in flight across the deploy fail closed once (one retry); a turn posted to a session owned by another user is refused with 404.Full detail in RELEASE_NOTES.md and CHANGELOG.md.
After merge
Squash-merge, then the backmerge
main→developis required — with a merge commit, not a squash.🤖 Generated with Claude Code