diff --git a/.github/workflows/platform.yml b/.github/workflows/platform.yml index 745b7ca3..5d80a24d 100644 --- a/.github/workflows/platform.yml +++ b/.github/workflows/platform.yml @@ -99,14 +99,13 @@ jobs: # http://localhost:4200 for a local SPA pointed at this deployment. # Empty on prod. Mirrors CDK_MCP_SANDBOX_EXTRA_FRAME_ANCESTORS below. CDK_ARTIFACTS_EXTRA_FRAME_ANCESTORS: ${{ vars.CDK_ARTIFACTS_EXTRA_FRAME_ANCESTORS }} - # "Shared with you" inbox on the artifact library. Default OFF, - # opt-in β€” the reverse of the flags above, because the surface ships - # ahead of the product decision about it. Unset resolves to an empty - # string, which config.ts treats as off; set the - # `CDK_ARTIFACT_SHARE_INBOX_ENABLED` variable to "true" in an - # environment to reveal it there. The fan-out rows the inbox reads are - # written regardless of this flag, so turning it on shows a complete - # inbox with no backfill. + # "Shared with you" inbox on the artifact library. Default ON with a + # kill switch, like the flags above: unset resolves to an empty string, + # which config.ts treats as "use the default (on)". Set the + # `CDK_ARTIFACT_SHARE_INBOX_ENABLED` variable to "false" in an + # environment to keep it dark there. The fan-out rows the inbox reads + # are written regardless of this flag, so toggling it never needs a + # backfill. CDK_ARTIFACT_SHARE_INBOX_ENABLED: ${{ vars.CDK_ARTIFACT_SHARE_INBOX_ENABLED }} CDK_FRONTEND_CERTIFICATE_ARN: ${{ vars.CDK_FRONTEND_CERTIFICATE_ARN }} # MCP Apps sandbox-proxy origin (mcp-sandbox.{domain}). Without the diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f7fc356..637c3473 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,30 @@ All notable changes to this project are documented in this file. Format follows For narrative release notes written for operators and product owners, see [RELEASE_NOTES.md](RELEASE_NOTES.md). +## [1.19.0] - 2026-09-06 + +A correctness release for interrupted turns, plus the share inbox coming out of the dark. Two separate defects made a conversation misreport its own history: a completed response could be labelled **"Response interrupted"** with a Continue button, and an interrupted one could show the model-directed `` in the user's own chat bubble β€” permanently. Both are fixed at the source rather than patched at the render. The artifact **"Shared with you" inbox now ships on by default** with a kill switch, so a fork gets the finished feature instead of having to discover a variable. Infrastructure adds `UserArtifactsIndex` to the existing `{prefix}-user-artifacts` table (one GSI operation) with a backfill for rows that predate it; nothing reads the index yet. **Requires a CDK deploy**, and two one-shot scripts are available post-deploy. + +### πŸš€ Added + +- **Open the full announcement from the banner.** The banner text is now a control that opens the announcement's modal, so a notice too long for one line is readable without hunting for the What's New panel (#987) +- **`UserArtifactsIndex`** on the existing `{prefix}-user-artifacts` table (`GSI2PK=USER#{uid}`, `GSI2SK=ARTIFACT#{updated_at}#{aid}`), sparse over HEAD rows, plus `scripts/backfill_artifact_user_index_keys.py` to stamp rows written before 2026-09-04. The artifact writer stamps the keys on both write paths. The library listing still reads the base table in this release (#982) +- **`scripts/backfill_false_interrupted_markers.py`** β€” one-shot cleanup for the stale interrupted-turn markers left by the defect below. Dry-run by default; clears only `navigated_away` markers written more than 900s after the session's last message (#988) + +### ⚠️ Changed + +- **The artifact "Shared with you" inbox now defaults ON** with a kill switch (`ARTIFACT_SHARE_INBOX_ENABLED=false` to disable), reversing the opt-in default it shipped with in 1.18.0. The surface landed before the product decision did; that decision has now been made, and an opt-in default would silently cost every fork a finished feature (#986) + +### πŸ› Fixed + +- **Completed responses were being labelled "Response interrupted"**, with a Continue button that would resume an already-finished answer. The SPA released a session's `AbortController` only on the Stop button, never on normal stream teardown, so every finished turn still looked in-flight for the life of the tab and the next refresh or tab close attributed a `navigated_away` interruption to it β€” one departure marking every session streamed in that tab. The controller is now released on teardown, and `POST /sessions/{id}/interrupt` verifies a turn is genuinely in flight against the session's single-flight lease before recording a departure (#988) +- **An interrupted turn showed the model-directed `` in the user's own chat bubble.** `displayText` β€” the clean copy of what the user typed β€” was written only by the stream coordinator's success path, so any turn that was stopped, dropped, or errored left the augmented prompt as the only text the UI could render. It is now written on `MessageAddedEvent`, when the user's message enters history and before the model call, so every exit path has it. Affected every prompt augmentation (RAG context, attachment guidance, MCP App context), not just interruption notes, and every model (#990) +- **The sidebar showed "No Chats Yet" while sessions were still loading.** The loading test read `value() === undefined`, but the resource short-circuits to `null` on the ordinary cold-start path β€” `SessionService` is constructed during `APP_INITIALIZER` before the BFF bootstrap resolves β€” and `reload()` preserves that `null` through the real fetch (#985) + +### πŸ—οΈ Infrastructure + +- New `UserArtifactsIndex` GSI on `{prefix}-user-artifacts`. **One** index operation on an existing table, within the DynamoDB `UpdateTable` limit; `infrastructure/gsi-inventory.json` records it (#982) + ## [1.18.0] - 2026-09-06 Artifacts stop being a per-conversation curiosity and become a place users go. There is a library at `/artifacts` with previews, rename, delete and an in-app viewer; artifacts can be **shared** with named people or the whole tenant; and sharing a conversation now shares the artifacts in it, which previously left the recipient staring at nothing where the owner saw cards. Alongside it, two more surfaces the platform had no way to do at all: **feature announcements** β€” an admin-authored What's New feed with a banner, a modal and per-announcement reach stats β€” and **mid-turn steering**, which lets a follow-up typed while the model is still working land inside the running turn at the next tool boundary instead of interrupting it. The SPA gains a **single-file rebranding surface** so a fork can change app name, greeting, logo and the entire color system without touching a component. On the cost side the GPT-5.6 family (Sol / Terra / Luna) is curated with verified rates, and every published GPT-5.6 rate in the catalog is corrected. **Requires a CDK deploy** (new `{prefix}-announcements` table, new IAM grants); no GSI operations on any existing table. diff --git a/README.md b/README.md index 79c393cf..d74ef909 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ **An open-source, production-ready Generative AI platform for institutions** *Built by Boise State University, designed for everyone.* -[![Release](https://img.shields.io/badge/Release-v1.18.0-6366f1?style=flat&logo=github&logoColor=white)](RELEASE_NOTES.md) +[![Release](https://img.shields.io/badge/Release-v1.19.0-6366f1?style=flat&logo=github&logoColor=white)](RELEASE_NOTES.md) [![Nightly](https://github.com/Boise-State-Development/agentcore-public-stack/actions/workflows/nightly.yml/badge.svg)](https://github.com/Boise-State-Development/agentcore-public-stack/actions/workflows/nightly.yml) ![Python](https://img.shields.io/badge/Python-3.13+-3776AB?style=flat&logo=python&logoColor=white) @@ -296,7 +296,7 @@ agentcore-public-stack/ See [RELEASE_NOTES.md](RELEASE_NOTES.md) for the full changelog, including new features, bug fixes, platform upgrades, and deployment notes for each release. -**Current release:** v1.18.0 +**Current release:** v1.19.0 --- diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 43898427..3bf62395 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -1,3 +1,76 @@ +# Release Notes β€” v1.19.0 + +**Release Date:** September 6, 2026 +**Previous Release:** v1.18.0 (September 6, 2026) + +--- + +> πŸ—οΈ **CDK deploy required.** One new GSI β€” `UserArtifactsIndex` on the **existing** `{prefix}-user-artifacts` table. That is **one** index operation on one existing table, within DynamoDB's one-GSI-per-`UpdateTable` limit, so this release does not need splitting. Confirm the index reports `ACTIVE` before considering the deploy done β€” CloudFormation reporting `UPDATE_COMPLETE` is not the same thing. +> +> βš™οΈ **`CDK_ARTIFACT_SHARE_INBOX_ENABLED` changes meaning.** It was opt-in (only `"true"` enabled the inbox); it is now a kill switch (unset or empty means **enabled**, only `"false"` disables). An environment that never set it **gains the "Shared with you" tab on this deploy**. Set it to `"false"` before deploying to keep that surface dark. +> +> 🧹 **Two optional one-shot scripts, both dry-run by default.** Neither is required for the deploy to be correct; see Deployment notes. + +--- + +## Highlights + +This release makes a conversation stop misreporting its own history. Two unrelated defects were doing it: a response that finished normally could be labelled **"Response interrupted"** with a Continue button that would resume an already-complete answer, and a response that genuinely *was* interrupted could show the model-directed `` β€” text written for the model, not the user β€” inside the user's own chat bubble, permanently. Both are fixed where they originate rather than papered over at the render layer. Alongside them, the artifact **"Shared with you" inbox now ships on by default** with a kill switch, and `UserArtifactsIndex` is added to the artifacts table as groundwork with nothing reading it yet. + +## Interrupted turns now tell the truth + +Two independent bugs, both surfacing as a conversation describing itself incorrectly after an interruption. + +**A completed turn could claim it was interrupted.** The SPA creates one `AbortController` per streaming request and used to release it only when the user clicked Stop β€” never on normal stream teardown. `streamingSessionIds()` reads exactly that controller to decide which turns a page departure interrupted, so after any turn that finished on its own, the session still looked in-flight for the rest of the tab's life. The next refresh, tab close, or cross-document navigation then attributed a `navigated_away` interruption to a finished answer, and a single departure marked *every* session that tab had ever streamed. On the next load the conversation showed "Response interrupted" and offered Continue, which would spend a turn resuming a response that had already ended. + +**An interrupted turn could show the note meant for the model.** When a turn is interrupted, the next turn's prompt carries an `` telling the model what happened. That note is deliberately part of persisted history β€” it is an honest record of what the model read β€” and the UI is supposed to render `displayText`, the clean copy of what the user typed, in its place. `displayText` was written by a single call site at the end of a *successful* turn, so a turn that was stopped, dropped, or errored never wrote one and the augmented prompt became the only text available to render. The failure concentrated where it was most visible: a turn only carries an interruption note because the *previous* turn was interrupted, so notes rode disproportionately on turns likely to be interrupted themselves. + +### Backend + +- `apis/app_api/sessions/routes.py` β€” `POST /sessions/{id}/interrupt` now verifies a turn is genuinely in flight against the session's single-flight lease before recording `navigated_away`. `user_stopped` is deliberately ungated: the Stop button only exists while streaming, and the same request arms distributed cancellation, which must still reach a turn whose lease read fails open. +- `agents/main_agent/session/hooks/display_text.py` β€” new `DisplayTextHook` writes `displayText` on `MessageAddedEvent`, when the user's message enters history and before the model call, so every later exit path already has it. Not every role-`user` message is the user: tool results carry that role, and Strands prepends a synthetic tool-result message ahead of the prompt when history ends on a dangling `toolUse` β€” precisely what an interrupted tool turn leaves behind β€” so content carrying `toolResult`/`toolUse` is skipped. +- `agents/main_agent/streaming/stream_coordinator.py` β€” arms the hook at the head of every turn, unconditionally including to `None`, because the agent instance is cached across turns. Its own end-of-turn write remains as a backstop for callers with no hook and for a failed write. + +### Frontend + +- `session/services/chat/chat-state.service.ts` and `chat-http.service.ts` β€” a session's controller is released on stream teardown, identity-checked so a superseded stream's late close cannot clear the controller of the stream that replaced it. + +### Test Coverage + +19 new backend tests across the hook and the coordinator's arming/backstop seam, plus SPA specs covering controller release, the identity guard, and the lease-gated and ungated interrupt paths. + +## πŸ› Bug fixes + +- **The sidebar said "No Chats Yet" while sessions were still loading**, showing an empty-state message to users who had conversations. The loading test read `value() === undefined`, but the sessions resource short-circuits to `null` on the ordinary cold-start path β€” `SessionService` is constructed during the `APP_INITIALIZER` pass, before the BFF bootstrap promise resolves β€” and `reload()` preserves that `null` through the real fetch, so the skeleton never rendered (#985) + +## ⚠️ Changed + +- **The artifact "Shared with you" inbox defaults on.** `ARTIFACT_SHARE_INBOX_ENABLED` was opt-in when the surface shipped in 1.18.0, because the surface landed before the product decision about it did. That decision has since been made and the inbox went live; leaving the default opt-in would mean every institution forking this repository silently loses a finished feature and has to discover a variable to get it back. `"false"` still turns it off (#986) + +## πŸ—οΈ Infrastructure + +- **`UserArtifactsIndex`** on the existing `{prefix}-user-artifacts` table β€” `GSI2PK=USER#{uid}`, `GSI2SK=ARTIFACT#{updated_at}#{aid}`, sparse over HEAD rows. The artifact writer stamps both keys on both of its write paths. **Nothing reads the index in this release**; the library listing still serves from the base table, so the index is groundwork and can be deployed and backfilled without any user-visible change (#982) + +## πŸš€ Deployment notes + +**A CDK deploy is required**, and one existing table gains one index. Verify it before moving on: + +```bash +aws dynamodb describe-table --table-name -user-artifacts \ + --query 'Table.GlobalSecondaryIndexes[].{Name:IndexName,Status:IndexStatus}' +``` + +`UPDATE_COMPLETE` on the stack does not mean the index is usable β€” wait for `ACTIVE`. + +**Check `CDK_ARTIFACT_SHARE_INBOX_ENABLED` before deploying.** Its default inverted: an environment that never set it gains the "Shared with you" tab. Set it to `"false"` first to keep that surface dark. + +**Two optional one-shot scripts**, both dry-run unless given `--apply`: + +- `backend/scripts/backfill_artifact_user_index_keys.py` β€” stamps `GSI2` keys onto artifact rows written before 2026-09-04. The index is sparse, so unstamped rows are absent from it rather than stale. Nothing reads the index yet, so this can run any time after it reports `ACTIVE` β€” but it must run before anything does. +- `backend/scripts/backfill_false_interrupted_markers.py` β€” clears interrupted-turn markers left by the defect above. It only touches `navigated_away` markers written more than 900s after the session's last message, which no interruption could have produced (the stream times out at 600s). Markers self-clear at the start of that session's next turn, so this only matters for conversations nobody returns to. + +--- + # Release Notes β€” v1.18.0 **Release Date:** September 6, 2026 diff --git a/VERSION b/VERSION index 84cc5294..815d5ca0 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.18.0 +1.19.0 diff --git a/backend/pyproject.toml b/backend/pyproject.toml index a2e25c75..3add5959 100644 --- a/backend/pyproject.toml +++ b/backend/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "agentcore-stack" -version = "1.18.0" +version = "1.19.0" requires-python = ">=3.10" description = "Multi-agent conversational AI system with AWS Bedrock AgentCore" readme = "README.md" diff --git a/backend/scripts/backfill_artifact_user_index_keys.py b/backend/scripts/backfill_artifact_user_index_keys.py new file mode 100644 index 00000000..2b843ecb --- /dev/null +++ b/backend/scripts/backfill_artifact_user_index_keys.py @@ -0,0 +1,222 @@ +"""Backfill: stamp GSI2PK/GSI2SK on artifact HEAD rows written before they existed. + +The artifact writer began stamping user-index keys on HEAD rows in +``feat(artifacts): stamp user-index keys on HEAD rows ahead of the index`` +(2026-09-04). Every HEAD row written before that carries neither +attribute: + + HEAD row : PK=USER#{user_id} SK=ARTIFACT#{aid}#HEAD + + GSI2PK=USER#{user_id} + + GSI2SK=ARTIFACT#{updated_at}#{aid} + +``UserArtifactsIndex`` is **sparse**: DynamoDB indexes a row only if it +carries the index's key attributes. A row missing them is not "stale" in +the index, it is *absent from it forever* β€” and the omission is silent. +Switching the library's user-wide listing to that index without this +backfill would drop every artifact created before 2026-09-04 from the +page, with no error anywhere. + +RUN THIS BEFORE THE INDEX IS CREATED +------------------------------------ +DynamoDB backfills a new GSI at creation time from rows that already +carry its keys. Stamping first therefore means the index is complete the +moment it reports ACTIVE, with no window in which it is partially +populated. The attributes are inert until an index consumes them (no +index write is charged), so running early costs nothing. + +WHAT IT DOES NOT TOUCH +---------------------- +``updated_at``. It is not a display field: HEAD's ``GSI1SK``/``GSI2SK`` +embed it, and only the writer maintains it. This script *reads* it to +build ``GSI2SK`` β€” matching byte-for-byte what the writer would have +written β€” and never assigns it. Same restraint as +``ArtifactLifecycleService.rename``. + +Version rows are skipped by design. The keys belong on HEAD rows only, +so the index holds one row per artifact rather than one per version. + +SAFETY +------ +* **Dry-run by default.** Pass ``--apply`` to write. +* **Idempotent.** Guarded by ``attribute_not_exists(GSI2PK)``, so a + second run finds nothing and a row the writer has since stamped is + left alone. +* **Never resurrects a deleted row.** ``attribute_exists(SK)`` on every + update, matching the writer's own write-back rule. +* **Reports what it cannot fix** rather than guessing β€” a HEAD row with + no ``updated_at`` is counted and named, not stamped with a fabricated + timestamp that would sort wrongly forever. + +Run against dev first, then prod:: + + AWS_PROFILE=dev-ai python backend/scripts/backfill_artifact_user_index_keys.py \\ + --table dev-boisestateai-v2-user-artifacts --region us-west-2 + AWS_PROFILE=dev-ai python backend/scripts/backfill_artifact_user_index_keys.py \\ + --table dev-boisestateai-v2-user-artifacts --region us-west-2 --apply +""" + +from __future__ import annotations + +import argparse +import logging +from typing import Any, Dict, Iterator, List + +import boto3 +from botocore.exceptions import ClientError + +logging.basicConfig( + level=logging.INFO, format="%(asctime)s %(levelname)s %(message)s" +) +logger = logging.getLogger("backfill_artifact_user_index_keys") + +HEAD_SUFFIX = "#HEAD" + + +def iter_head_rows(table: Any) -> Iterator[Dict[str, Any]]: + """Every artifact HEAD row in the table. + + A Scan, not a Query: this is a whole-table migration across all + users, and the table's partition key is the user. The + ``FilterExpression`` runs server-side purely to cut payload β€” + DynamoDB still reads every item either way, so it saves bandwidth, + not capacity. + """ + kwargs: Dict[str, Any] = { + "FilterExpression": "begins_with(SK, :p)", + "ExpressionAttributeValues": {":p": "ARTIFACT#"}, + } + while True: + resp = table.scan(**kwargs) + for item in resp.get("Items", []): + if str(item.get("SK", "")).endswith(HEAD_SUFFIX): + yield item + last = resp.get("LastEvaluatedKey") + if not last: + return + kwargs["ExclusiveStartKey"] = last + + +def plan_row(item: Dict[str, Any]) -> Dict[str, Any] | None: + """What this row needs, or None if it needs nothing. + + Returns a dict with the computed keys, or ``{"skip": reason}`` for a + row that cannot be stamped safely. + """ + if "GSI2PK" in item: + return None # already stamped β€” writer or a previous run + + pk = str(item.get("PK", "")) + artifact_id = str(item.get("artifact_id", "")) + updated_at = str(item.get("updated_at", "")) + + if not pk.startswith("USER#"): + return {"skip": f"unexpected PK {pk!r}"} + if not artifact_id: + return {"skip": "no artifact_id attribute"} + if not updated_at: + # Deliberately not falling back to created_at or "now": GSI2SK is + # the sort key the library orders by, and a fabricated timestamp + # would order this artifact wrongly for the rest of its life. + # Better to name it and let a human decide. + return {"skip": "no updated_at attribute"} + + return { + "gsi2pk": pk, # GSI2PK is exactly the base PK β€” USER#{user_id} + "gsi2sk": f"ARTIFACT#{updated_at}#{artifact_id}", + } + + +def backfill(table: Any, apply: bool) -> Dict[str, int]: + stats = {"head_rows": 0, "already": 0, "stamped": 0, "skipped": 0, "failed": 0} + skipped: List[str] = [] + + for item in iter_head_rows(table): + stats["head_rows"] += 1 + plan = plan_row(item) + + if plan is None: + stats["already"] += 1 + continue + + sk = str(item.get("SK", "")) + if "skip" in plan: + stats["skipped"] += 1 + skipped.append(f"{item.get('PK')} / {sk}: {plan['skip']}") + continue + + logger.info( + "stamp %s %s -> GSI2SK=%s", item.get("PK"), sk, plan["gsi2sk"] + ) + if not apply: + stats["stamped"] += 1 + continue + + try: + table.update_item( + Key={"PK": item["PK"], "SK": item["SK"]}, + UpdateExpression="SET GSI2PK = :pk, GSI2SK = :sk", + ExpressionAttributeValues={ + ":pk": plan["gsi2pk"], + ":sk": plan["gsi2sk"], + }, + # attribute_exists(SK): never resurrect a row the delete + # path removed between the scan and this write. + # attribute_not_exists(GSI2PK): idempotent, and yields to + # the writer if it stamped the row in the meantime. + ConditionExpression=( + "attribute_exists(SK) AND attribute_not_exists(GSI2PK)" + ), + ) + stats["stamped"] += 1 + except ClientError as exc: + code = exc.response.get("Error", {}).get("Code", "") + if code == "ConditionalCheckFailedException": + # Deleted, or stamped by the writer, while we scanned. + stats["already"] += 1 + continue + stats["failed"] += 1 + logger.error("failed to stamp %s: %s", sk, code, exc_info=True) + + if skipped: + logger.warning("%s row(s) could not be stamped:", len(skipped)) + for line in skipped: + logger.warning(" %s", line) + + return stats + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--table", required=True, help="user-artifacts table name") + parser.add_argument("--region", default="us-west-2") + parser.add_argument( + "--apply", + action="store_true", + help="actually write (default is a dry run)", + ) + args = parser.parse_args() + + table = boto3.resource("dynamodb", region_name=args.region).Table(args.table) + + if not args.apply: + logger.info("DRY RUN β€” no writes. Pass --apply to commit.") + + stats = backfill(table, args.apply) + + logger.info( + "HEAD rows=%s already-stamped=%s stamped=%s skipped=%s failed=%s", + stats["head_rows"], + stats["already"], + stats["stamped"], + stats["skipped"], + stats["failed"], + ) + if stats["skipped"] or stats["failed"]: + logger.warning( + "Index will be INCOMPLETE for the rows above. Resolve them " + "before switching the library query to UserArtifactsIndex." + ) + + +if __name__ == "__main__": + main() diff --git a/backend/scripts/backfill_false_interrupted_markers.py b/backend/scripts/backfill_false_interrupted_markers.py new file mode 100644 index 00000000..eebec127 --- /dev/null +++ b/backend/scripts/backfill_false_interrupted_markers.py @@ -0,0 +1,221 @@ +#!/usr/bin/env python3 +"""Clear interrupted-turn markers that no interruption could have produced. + +WHY THIS EXISTS +Until PR #988 the SPA released a session's ``AbortController`` only on the +Stop button, never on normal stream teardown. ``streamingSessionIds()`` reads +that controller to decide which turns a page departure interrupted, so every +completed turn stayed "in flight" for the life of the tab and the next +refresh / tab close / navigation POSTed ``navigated_away`` for it β€” one +departure marking every session ever streamed in that tab. + +A stale marker costs twice. It shows a "Response interrupted" chip plus a +Continue button on a complete answer (and Continue bills a +``continue_truncated`` turn that resumes an already-finished message), and it +makes the session's NEXT prompt carry a false ```` β€” +persisted in history, invisible in the UI, telling the model its previous +response was cut off. Markers self-clear only at the start of that session's +next turn (``clear_interrupted_turn``), so a conversation nobody returns to +stays armed indefinitely. + +WHY THE 900s THRESHOLD, AND NOT "THE MARKER LANDED AFTER THE LAST MESSAGE" +A genuine interruption does not always bump ``lastMessageAt``: the +"marker only, no synthetic write" branch of ``_persist_interruption`` (an +interrupted continuation, where the history tail is already an assistant +turn) leaves it at the previous turn. So a modest positive gap is ambiguous +and clearing on it would erase real interruptions. + +What is NOT ambiguous is a gap wider than a turn can live. The SSE stream +times out at 600s, so no turn is still running 15 minutes after its last +message β€” a departure signal that lands then cannot have interrupted +anything. That is the only claim this script acts on. + +SAFETY +* Dry-run by default; ``--apply`` is required to write. +* Only ``navigated_away`` rows are considered. ``user_stopped`` is the user's + own attested intent and ``connection_lost`` comes from the server's own + backstop β€” neither is this bug, and neither is touched. +* Each write is conditional on the exact ``lastTurnInterruptedAt`` / + ``lastTurnInterruptReason`` the scan read, so a session that started a new + turn (and was legitimately re-marked) between scan and write is skipped + rather than clobbered. +* Idempotent: a second run finds nothing left to do. + +USAGE + python scripts/backfill_false_interrupted_markers.py \ + --table boisestateai-v2-sessions-metadata --profile prod-ai + # …review the dry-run summary, then: + python scripts/backfill_false_interrupted_markers.py \ + --table boisestateai-v2-sessions-metadata --profile prod-ai --apply +""" + +from __future__ import annotations + +import argparse +import sys +from datetime import datetime, timezone +from typing import Any, Iterator, Optional + +import boto3 +from botocore.exceptions import ClientError + +# The SSE stream times out at 600s. A departure signal that lands more than +# this long after the session's last message cannot have interrupted a live +# turn, whatever the history tail looks like. The extra margin over 600s is +# deliberate slack for clock skew and teardown latency. +DEFAULT_MIN_GAP_SECONDS = 900 + +MARKER_ATTRS = ("lastTurnInterrupted", "lastTurnInterruptReason", "lastTurnInterruptedAt") + + +def _parse_iso(value: Optional[str]) -> Optional[datetime]: + """Parse a stored ISO-8601 timestamp, tolerating a missing offset.""" + if not isinstance(value, str): + return None + try: + parsed = datetime.fromisoformat(value) + except ValueError: + return None + return parsed if parsed.tzinfo else parsed.replace(tzinfo=timezone.utc) + + +def _scan_marked_rows(table: Any) -> Iterator[dict]: + """Yield every session row carrying an interrupted-turn marker.""" + kwargs: dict[str, Any] = { + "FilterExpression": "attribute_exists(#lti)", + "ProjectionExpression": "#pk, #sk, #gsipk, #lti, #ltr, #ltia, #lma", + "ExpressionAttributeNames": { + "#pk": "PK", + "#sk": "SK", + "#gsipk": "GSI_PK", + "#lti": "lastTurnInterrupted", + "#ltr": "lastTurnInterruptReason", + "#ltia": "lastTurnInterruptedAt", + "#lma": "lastMessageAt", + }, + } + while True: + response = table.scan(**kwargs) + yield from response.get("Items", []) + last_key = response.get("LastEvaluatedKey") + if not last_key: + return + kwargs["ExclusiveStartKey"] = last_key + + +def _select(items: list[dict], min_gap_seconds: int) -> tuple[list[dict], dict[str, int]]: + """Split scanned rows into the provably-false ones and a reason tally.""" + selected: list[dict] = [] + skipped = {"other_reason": 0, "gap_too_small": 0, "unparseable": 0} + + for item in items: + if item.get("lastTurnInterruptReason") != "navigated_away": + skipped["other_reason"] += 1 + continue + marked_at = _parse_iso(item.get("lastTurnInterruptedAt")) + last_message_at = _parse_iso(item.get("lastMessageAt")) + if marked_at is None or last_message_at is None: + skipped["unparseable"] += 1 + continue + gap = (marked_at - last_message_at).total_seconds() + if gap <= min_gap_seconds: + skipped["gap_too_small"] += 1 + continue + selected.append({**item, "_gapSeconds": gap}) + + selected.sort(key=lambda row: row["_gapSeconds"], reverse=True) + return selected, skipped + + +def _clear(table: Any, row: dict) -> str: + """Remove one row's marker. Returns 'cleared', 'raced', or 'error'.""" + try: + table.update_item( + Key={"PK": row["PK"], "SK": row["SK"]}, + UpdateExpression="REMOVE #lti, #ltr, #ltia", + ExpressionAttributeNames={ + "#lti": "lastTurnInterrupted", + "#ltr": "lastTurnInterruptReason", + "#ltia": "lastTurnInterruptedAt", + }, + # The row must still be exactly what the scan saw. A session that + # ran a new turn in between has either cleared the marker itself + # or been re-marked by a real interruption; both must be left + # alone. + ConditionExpression=( + "attribute_exists(#lti) AND #ltia = :ts AND #ltr = :reason" + ), + ExpressionAttributeValues={ + ":ts": row["lastTurnInterruptedAt"], + ":reason": "navigated_away", + }, + ) + return "cleared" + except ClientError as e: + if e.response.get("Error", {}).get("Code") == "ConditionalCheckFailedException": + return "raced" + print(f" ERROR on {row.get('GSI_PK', row['SK'])}: {e}", file=sys.stderr) + return "error" + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + parser.add_argument("--table", required=True, help="Sessions-metadata DynamoDB table name") + parser.add_argument("--profile", default=None, help="AWS profile (default: ambient credentials)") + parser.add_argument("--region", default="us-west-2") + parser.add_argument( + "--min-gap-seconds", + type=int, + default=DEFAULT_MIN_GAP_SECONDS, + help=( + "Only clear markers written more than this long after the session's " + f"last message (default {DEFAULT_MIN_GAP_SECONDS}; must exceed the " + "600s stream timeout to stay provably false)" + ), + ) + parser.add_argument("--apply", action="store_true", help="Write. Without it, dry-run only.") + args = parser.parse_args() + + if args.min_gap_seconds <= 600: + parser.error( + "--min-gap-seconds must exceed the 600s stream timeout; below that a " + "marker can belong to a turn that was genuinely still running." + ) + + session = boto3.Session(profile_name=args.profile, region_name=args.region) + table = session.resource("dynamodb").Table(args.table) + + print(f"Scanning {args.table} ({args.region}) for interrupted-turn markers…") + items = list(_scan_marked_rows(table)) + selected, skipped = _select(items, args.min_gap_seconds) + + print(f"\n rows carrying a marker: {len(items)}") + print(f" left alone β€” not navigated_away: {skipped['other_reason']}") + print(f" left alone β€” gap <= {args.min_gap_seconds}s:{'':<9}{skipped['gap_too_small']}") + if skipped["unparseable"]: + print(f" left alone β€” unparseable dates: {skipped['unparseable']}") + print(f" provably false, to clear: {len(selected)}") + + if selected: + print("\n widest gaps:") + for row in selected[:5]: + days = row["_gapSeconds"] / 86400 + print(f" {row.get('GSI_PK', row['SK'])} +{days:.1f}d after last message") + + if not args.apply: + print("\nDRY RUN β€” nothing written. Re-run with --apply to clear.") + return 0 + + print(f"\nClearing {len(selected)} markers…") + tally = {"cleared": 0, "raced": 0, "error": 0} + for row in selected: + tally[_clear(table, row)] += 1 + + print(f"\n cleared: {tally['cleared']}") + print(f" skipped (row changed under us): {tally['raced']}") + print(f" errors: {tally['error']}") + return 1 if tally["error"] else 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/backend/src/agents/builtin_tools/artifacts/service.py b/backend/src/agents/builtin_tools/artifacts/service.py index 71e16ff6..f5b8bf98 100644 --- a/backend/src/agents/builtin_tools/artifacts/service.py +++ b/backend/src/agents/builtin_tools/artifacts/service.py @@ -10,7 +10,7 @@ + GSI1PK=SESSION#{session_id} + GSI1SK=ARTIFACT#{updated_at}#{aid} (SessionIndex) + GSI2PK=USER#{user_id} - + GSI2SK=ARTIFACT#{updated_at}#{aid} (index NOT YET CREATED) + + GSI2SK=ARTIFACT#{updated_at}#{aid} (UserArtifactsIndex) S3 layout : {user_id}/{aid}/v{n}/index.html Versions are immutable (no DeleteObject grant in inference-api) β€” an @@ -27,20 +27,19 @@ (the optimistic lock below) nor `updated_at` (embedded in the GSI sort keys, which only this module maintains). -GSI2PK/GSI2SK are written ahead of the index that will consume them. -Nothing queries them today β€” a user-wide artifact list is served by a -base-table Query on PK=USER#{uid}, which is adequate while the heaviest -user holds well under a 1MB page. They are stamped now because a sparse -GSI only ever contains rows that already carry its key attributes: rows -written before the attributes exist stay invisible to it forever unless -a migration script backfills them, and that omission fails silently (a -library page that lists only artifacts created after the deploy, with no -error anywhere). Writing them from now on means the eventual -`UserArtifactsIndex` β€” GSI2PK hash, GSI2SK range, queried with -ScanIndexForward=False for newest-first β€” backfills every row stamped -since this change, shrinking the manual backfill to the rows that -predate it. Until then they are inert ordinary attributes: DynamoDB -charges no index write when no index consumes them. +GSI2PK/GSI2SK feed `UserArtifactsIndex` β€” GSI2PK hash, GSI2SK range, +queried with ScanIndexForward=False for newest-first. They were stamped +here for two weeks before that index existed, deliberately: a sparse GSI +only ever contains rows that already carry its key attributes, so rows +written before the attributes exist stay invisible to it forever, and +that omission fails silently (a library page listing only artifacts +created after the deploy, with no error anywhere). Stamping early shrank +the manual backfill to the rows predating 2026-09-04, which +`backend/scripts/backfill_artifact_user_index_keys.py` then stamped. + +**Both write paths below must keep stamping them.** A HEAD row that +loses these attributes on its next update drops out of the index β€” and +out of the owner's library β€” with nothing to indicate it. Stamped on HEAD rows only, deliberately: one indexed row per artifact rather than one per version. Both write paths below must stamp them, or diff --git a/backend/src/agents/main_agent/base_agent.py b/backend/src/agents/main_agent/base_agent.py index e87d4ba1..15a29fe0 100644 --- a/backend/src/agents/main_agent/base_agent.py +++ b/backend/src/agents/main_agent/base_agent.py @@ -13,6 +13,7 @@ from agents.main_agent.core import ModelConfig, SystemPromptBuilder, AgentFactory from agents.main_agent.session import SessionFactory from agents.main_agent.session.hooks import ( + DisplayTextHook, SteeringHook, StopHook, OAuthConsentHook, @@ -276,6 +277,9 @@ def _create_hooks(self) -> List: - StopHook: Always enabled, cancels tool execution on user stop - SteeringHook: Injects a follow-up queued mid-turn at the next tool boundary + - DisplayTextHook: Stores the user's original message for UI display + as soon as their turn is appended, so an augmented prompt is never + what the UI renders for an interrupted turn - OAuthConsentHook: Pauses the agent (Strands interrupt) when an OAuth-gated MCP tool is about to run without a cached token - Approval hooks: Gate dangerous operations for user confirmation @@ -300,6 +304,15 @@ def _create_hooks(self) -> List: self.steering_hook = SteeringHook(self.session_manager) hooks.append(self.steering_hook) + # Persist the user's own words (`displayText`) the moment their turn + # enters history, so an augmented prompt β€” RAG context, attachment + # guidance, an `` β€” never becomes what the UI shows + # for a turn that doesn't finish. Held on the wrapper so the stream + # coordinator can arm it per turn and skip its own end-of-turn write + # once this has done it. + self.display_text_hook = DisplayTextHook() + hooks.append(self.display_text_hook) + # OAuth consent gate for external MCP tools. Registered unconditionally; # the hook is a no-op for tools that don't have a registered provider. hooks.append(self._build_oauth_consent_hook()) diff --git a/backend/src/agents/main_agent/session/hooks/__init__.py b/backend/src/agents/main_agent/session/hooks/__init__.py index 840944ff..0251a49a 100644 --- a/backend/src/agents/main_agent/session/hooks/__init__.py +++ b/backend/src/agents/main_agent/session/hooks/__init__.py @@ -1,6 +1,7 @@ """Hooks for Main Agent""" from agents.main_agent.session.hooks.context_attribution import ContextAttributionHook +from agents.main_agent.session.hooks.display_text import DisplayTextHook from agents.main_agent.session.hooks.oauth_consent import OAuthConsentHook from agents.main_agent.session.hooks.prefix_fingerprint import PrefixFingerprintHook from agents.main_agent.session.hooks.steering import SteeringHook @@ -9,6 +10,7 @@ __all__ = [ "ContextAttributionHook", + "DisplayTextHook", "OAuthConsentHook", "PrefixFingerprintHook", "SteeringHook", diff --git a/backend/src/agents/main_agent/session/hooks/display_text.py b/backend/src/agents/main_agent/session/hooks/display_text.py new file mode 100644 index 00000000..268aeb90 --- /dev/null +++ b/backend/src/agents/main_agent/session/hooks/display_text.py @@ -0,0 +1,147 @@ +"""Persist the user's own words as soon as their turn enters history. + +The prompt that reaches the model is often not the prompt the user typed. RAG +prepends retrieved context, attachments add guidance, an embedded MCP App +pushes a context block, and an interrupted previous turn prepends an +```` addressed to the model. All of that is deliberately +kept in persisted history β€” it is an honest record of what the model actually +read β€” and the UI is supposed to show the clean original instead, via the +``displayText`` (``D#``) record this hook writes. + +**Why a hook, and why this event.** That write used to live at the very end of +``stream_coordinator.stream_response``, in the success path. Nothing on the +Stop, disconnect, or error paths wrote it, so any turn that did not reach that +final line left the raw augmented prompt as the only thing the UI could +render β€” and a turn is at its most likely to be interrupted precisely when it +is carrying an interruption note, because the note only exists because the +*previous* turn was interrupted. The visible result was the model-directed +note sitting in the user's own chat bubble, permanently. It also showed +transiently on any reload mid-turn, for every augmentation. + +``MessageAddedEvent`` fires from ``Agent._append_messages``, the moment the +user's turn is really in the conversation β€” before the model call, so every +later exit path (completion, Stop, cancellation, error, container death) +already has the record written. That is the whole point: the write no longer +depends on how the turn ends. + +**Why not simply write at request start.** If the turn died before the user +message was appended, a record keyed to that index would be inherited by +whatever message later takes the index β€” showing one turn's clean text on a +different turn's bubble. Anchoring to the actual append makes the index and +the record land together. + +**One-shot per turn, and why role alone is not enough.** Tool-result messages +are also role ``user`` under Bedrock Converse, and mid-turn steering appends +into them. The hook is armed once per turn and disarms on the first user-role +message it writes, which is the user's prompt β€” tool results only exist after +the first model call. + +Armed unconditionally at the head of every turn, *including to ``None``*, for +the same reason ``turn_lease`` is stamped unconditionally: the agent instance +is cached and outlives the turn (#741/#751), so an arm left behind by a +previous turn would fire against the wrong one. +""" + +from __future__ import annotations + +import logging +from typing import Any, Optional + +from strands.hooks import HookProvider, HookRegistry, MessageAddedEvent + +logger = logging.getLogger(__name__) + + +class DisplayTextHook(HookProvider): + """Write the turn's ``displayText`` when the user message is appended. + + Best-effort in every direction: ``displayText`` is a UI nicety, and a + failure here must never break a turn. When it does fail, the stream + coordinator's end-of-turn write is still there as a backstop for turns + that complete. + """ + + def __init__(self) -> None: + self._armed: Optional[dict] = None + self._written = False + + def register_hooks(self, registry: HookRegistry, **kwargs: Any) -> None: + registry.add_callback(MessageAddedEvent, self.write_display_text) + + @property + def wrote_this_turn(self) -> bool: + """Whether this turn's record is already stored. + + Read by the stream coordinator so its end-of-turn backstop doesn't + repeat a write this hook already made. + """ + return self._written + + def arm( + self, + *, + session_id: str, + user_id: str, + message_index: int, + display_text: Optional[str], + ) -> None: + """Prime the hook for one turn, or clear it when there's nothing to write. + + ``display_text`` is the user's original message, passed only when the + prompt was modified before reaching the model. A turn that sends the + user's text verbatim (and a resume / continuation, which sends no new + user turn at all) passes ``None`` and disarms. + """ + self._written = False + if not display_text: + self._armed = None + return + self._armed = { + "session_id": session_id, + "user_id": user_id, + "message_id": message_index, + "display_text": display_text, + } + + async def write_display_text(self, event: MessageAddedEvent) -> None: + """Store the clean text once the user's message is in history.""" + armed = self._armed + if armed is None: + return + + message = getattr(event, "message", None) or {} + if message.get("role") != "user": + return + # Not every role-`user` message is the user speaking. Tool results + # carry that role under Bedrock Converse, and Strands prepends a + # SYNTHETIC tool-result message ahead of the prompt when history ends + # on a dangling `toolUse` (`Agent._run_loop`, "appending a toolResult + # message to have valid conversation") β€” which is precisely the shape + # an interrupted tool turn leaves behind, i.e. the case this hook + # exists for. Consuming the arm there would stamp the clean text onto + # the repair message instead of the user's own. + if any( + isinstance(block, dict) and ("toolResult" in block or "toolUse" in block) + for block in message.get("content") or [] + ): + return + + # One-shot: consume before the await so a tool-result message later in + # the same turn can never re-enter this. + self._armed = None + + try: + from apis.shared.sessions.metadata import store_user_display_text + + await store_user_display_text(**armed) + self._written = True + logger.info( + "πŸ’Ύ Stored displayText for user message %s at append time", + armed["message_id"], + ) + except Exception: # noqa: BLE001 - a UI nicety must never break a turn + logger.error( + "Failed to store displayText for user message %s", + armed["message_id"], + exc_info=True, + ) diff --git a/backend/src/agents/main_agent/streaming/stream_coordinator.py b/backend/src/agents/main_agent/streaming/stream_coordinator.py index ac644d2e..6b954c04 100644 --- a/backend/src/agents/main_agent/streaming/stream_coordinator.py +++ b/backend/src/agents/main_agent/streaming/stream_coordinator.py @@ -344,6 +344,22 @@ async def stream_response( initial_message_count = self._get_initial_message_count(session_manager) logger.info(f"πŸ“Š Initial message count before streaming: {initial_message_count}") + # Arm the displayText write for this turn. The hook stores the user's + # original message on `MessageAddedEvent` β€” i.e. before the model + # call β€” so a turn that is stopped, dropped, or errors still has the + # clean text to render instead of the augmented prompt the model was + # sent. Armed UNCONDITIONALLY, including to None: the agent instance + # is cached across turns (#741/#751), so an arm left by a previous + # turn would otherwise fire against this one. See the end-of-turn + # backstop below for wrappers that carry no hook. + self._arm_display_text( + main_agent_wrapper, + session_id=session_id, + user_id=user_id, + message_index=initial_message_count, + display_text=original_message, + ) + # MCP Apps PR #5: subscribe this conversation stream to the # app-initiated tool-event broker so a `tools/call` proxied from an # embedded MCP App surfaces as a tool_use/tool_result card in the @@ -1186,8 +1202,13 @@ async def stream_response( logger.info(f"βœ… Message metadata stored for {len(message_ids_to_store)} assistant messages (sequential)") - # Store displayText for user message if original_message differs from augmented - if original_message: + # displayText backstop. `DisplayTextHook` normally wrote this at + # append time, which is the write that matters β€” it is the only + # one an interrupted turn ever reaches. This runs only when that + # didn't happen: a wrapper with no hook (voice, tests), or a + # failed write. Skipped otherwise, so the normal path still makes + # exactly one put. + if original_message and not self._display_text_written(main_agent_wrapper): user_message_index = initial_message_count # User message is first in this turn try: from apis.shared.sessions.metadata import store_user_display_text @@ -2136,6 +2157,48 @@ def _emit_tool_input_partial( logger.warning("Failed to emit ui_tool_input_partial event: %s", e) return [] + def _arm_display_text( + self, + main_agent_wrapper: Any, + *, + session_id: str, + user_id: str, + message_index: int, + display_text: Optional[str], + ) -> None: + """Prime this turn's ``displayText`` write on the agent's hook. + + No-op for a wrapper that carries no hook (voice, tests) β€” those fall + through to the coordinator's end-of-turn backstop, which is exactly + the behaviour they had before the hook existed. + """ + hook = getattr(main_agent_wrapper, "display_text_hook", None) + if hook is None: + return + try: + hook.arm( + session_id=session_id, + user_id=user_id, + message_index=message_index, + display_text=display_text, + ) + except Exception: # noqa: BLE001 - never break a turn on a UI nicety + logger.warning("Could not arm displayText hook", exc_info=True) + + def _display_text_written(self, main_agent_wrapper: Any) -> bool: + """Whether the hook already stored this turn's ``displayText``. + + False whenever we can't tell, so the backstop runs β€” a duplicate put + of an identical record is harmless, a missing one is the bug. + """ + hook = getattr(main_agent_wrapper, "display_text_hook", None) + if hook is None: + return False + try: + return bool(hook.wrote_this_turn) + except Exception: # noqa: BLE001 + return False + def _drain_steering_events( self, main_agent_wrapper: Any, session_id: str ) -> List[str]: diff --git a/backend/src/apis/app_api/sessions/routes.py b/backend/src/apis/app_api/sessions/routes.py index 7567b665..69ad4e80 100644 --- a/backend/src/apis/app_api/sessions/routes.py +++ b/backend/src/apis/app_api/sessions/routes.py @@ -685,7 +685,9 @@ async def signal_turn_interrupted_endpoint( user left; they did not reject anything. **Recorded only** β€” the running turn is deliberately left alone, matching today's behaviour where a refresh lets the turn finish server-side and the reload - offers to continue it. + offers to continue it. Recorded only while the session's + single-flight lease is held: "mid-turn" is a claim this endpoint + verifies rather than takes from the client (see the gate below). Lives on app-api, not inference-api: the AgentCore Runtime data plane only proxies ``/invocations`` + ``/ping``, so a custom inference-api @@ -703,6 +705,29 @@ async def signal_turn_interrupted_endpoint( logger.info("POST /sessions/.../interrupt (reason=%s)", body.reason) try: + # A departure can only interrupt a turn that is actually running. + # The SPA decides that from its own transport state, which has been + # wrong before: a controller left behind after a completed stream + # made every finished turn in the tab eligible, so a later refresh + # marked complete answers as interrupted (a false "Response + # interrupted" chip, and a false interruption note on the session's + # next prompt). The single-flight lease is the server's own answer to + # "is a turn in flight", so assert it here rather than trusting the + # client β€” old tabs keep running the old SPA long after the fix ships. + # + # `user_stopped` is deliberately NOT gated: the Stop button only + # exists while streaming, and it also arms cancellation below, which + # must reach a turn whose lease read fails for any reason. + if body.reason == "navigated_away": + from apis.shared.sessions.session_lease import is_session_lease_held + + if not await is_session_lease_held(session_id, user_id): + logger.info( + "Ignoring navigated_away for session %s β€” no turn in flight", + session_id, + ) + return Response(status_code=204) + await set_interrupted_turn( session_id, user_id, diff --git a/backend/src/apis/shared/feature_flags.py b/backend/src/apis/shared/feature_flags.py index 5681a5e6..d9e8e974 100644 --- a/backend/src/apis/shared/feature_flags.py +++ b/backend/src/apis/shared/feature_flags.py @@ -197,13 +197,23 @@ def artifact_share_inbox_enabled() -> bool: """Whether a recipient can *discover* artifacts shared with them. Covers the ``GET /shared-artifacts`` inbox endpoint and, through it, - the library page's "Shared with you" tab. **Default OFF, opt-in** - (the deferred-feature pattern, mirroring the long-deleted - ``FINE_TUNING_ENABLED``): only the literal ``"true"`` - (case-insensitive) enables it. Every other flag in this module ships - default-on with a kill switch; this one is deliberately the other - way round, because the surface it gates lands before the product - decision about it does. + the library page's "Shared with you" tab. **Default ON with a kill + switch** (house style, mirroring ``announcements_enabled`` and + ``scheduled_runs_enabled``): unset or empty resolves to enabled; only + the literal ``"false"`` (case-insensitive) disables. + + It shipped the other way round β€” default off, opt-in β€” because the + surface landed before the product decision about it did. That + decision was made in 1.18.0 and the inbox went live; carrying an + opt-in default past it would mean every institution forking this + repo silently loses a finished feature, and has to discover a + variable to get it back. Default-on is the right answer for a fork, + and ``"false"`` still turns it off for anyone who wants it dark. + + Note the empty-string case is load-bearing in the *opposite* + direction now: an unset GitHub Actions variable forwards ``""``, + which under this flag means **on**. That is deliberate β€” a fork that + never sets the variable is exactly who this default is for. ############################################################ # This flag gates the READ ONLY. The recipient fan-out rows the @@ -222,5 +232,5 @@ def artifact_share_inbox_enabled() -> bool: """ return ( os.environ.get("ARTIFACT_SHARE_INBOX_ENABLED", "").strip().lower() - == "true" + != "false" ) diff --git a/backend/tests/agents/main_agent/session/test_display_text_hook.py b/backend/tests/agents/main_agent/session/test_display_text_hook.py new file mode 100644 index 00000000..8a5d6dc3 --- /dev/null +++ b/backend/tests/agents/main_agent/session/test_display_text_hook.py @@ -0,0 +1,226 @@ +"""Tests for DisplayTextHook β€” persist the user's own words at append time. + +The bug this exists to close: `displayText` used to be written only by the +stream coordinator's success path, so any turn that was stopped, dropped, or +errored left the *augmented* prompt as the only thing the UI could render. +That put a model-directed `` in the user's own chat bubble, +permanently β€” and it landed most often on exactly the turns carrying such a +note, since the note only exists because the previous turn was interrupted. + +So the properties under test, in order of how expensive they are to get wrong: + +1. **The write happens on append, before the model call.** That is what makes + it independent of how the turn ends. +2. **One-shot per turn.** Tool-result messages are role `user` too; a second + write would relabel the wrong message index. +3. **No stale arm.** The agent instance is cached across turns (#741/#751), so + an un-armed turn must never inherit the previous turn's text. +4. Fail-soft: a storage failure never propagates into the turn. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from strands.hooks import MessageAddedEvent + +from agents.main_agent.session.hooks.display_text import DisplayTextHook + + +def _user_message(text: str = "hi"): + return {"role": "user", "content": [{"text": text}]} + + +def _assistant_message(): + return {"role": "assistant", "content": [{"text": "answer"}]} + + +def _tool_result_message(): + return { + "role": "user", + "content": [{"toolResult": {"toolUseId": "t1", "content": [{"text": "ok"}]}}], + } + + +def _event(message): + return MessageAddedEvent(agent=MagicMock(), message=message) + + +@pytest.fixture +def armed_hook(): + hook = DisplayTextHook() + hook.arm( + session_id="s1", + user_id="u1", + message_index=4, + display_text="what the user actually typed", + ) + return hook + + +@pytest.fixture +def store(): + with patch( + "apis.shared.sessions.metadata.store_user_display_text", new_callable=AsyncMock + ) as mock: + yield mock + + +class TestWritesOnAppend: + @pytest.mark.asyncio + async def test_stores_the_original_text_when_the_user_message_lands( + self, armed_hook, store + ): + await armed_hook.write_display_text(_event(_user_message())) + + store.assert_awaited_once_with( + session_id="s1", + user_id="u1", + message_id=4, + display_text="what the user actually typed", + ) + assert armed_hook.wrote_this_turn is True + + @pytest.mark.asyncio + async def test_ignores_assistant_messages(self, armed_hook, store): + await armed_hook.write_display_text(_event(_assistant_message())) + + store.assert_not_awaited() + # Still armed β€” the user turn hasn't landed yet. + assert armed_hook.wrote_this_turn is False + + @pytest.mark.asyncio + async def test_writes_once_even_though_tool_results_are_role_user( + self, armed_hook, store + ): + """Under Bedrock Converse a tool-result message is role `user` too. + + A second write would stamp this turn's clean text onto a message index + that isn't the user's prompt. + """ + await armed_hook.write_display_text(_event(_user_message())) + await armed_hook.write_display_text(_event(_tool_result_message())) + await armed_hook.write_display_text(_event(_tool_result_message())) + + assert store.await_count == 1 + + @pytest.mark.asyncio + async def test_a_synthetic_tool_result_repair_does_not_consume_the_arm( + self, armed_hook, store + ): + """Strands prepends a role-`user` tool-result message ahead of the + prompt when history ends on a dangling `toolUse` (agent.py, "appending + a toolResult message to have valid conversation"). + + That is exactly the shape an interrupted tool turn leaves behind β€” the + case this hook exists for β€” so consuming the arm there would stamp the + clean text onto the repair message and leave the user's own prompt + showing the augmented text. + """ + await armed_hook.write_display_text(_event(_tool_result_message())) + store.assert_not_awaited() + + await armed_hook.write_display_text(_event(_user_message())) + + store.assert_awaited_once_with( + session_id="s1", + user_id="u1", + message_id=4, + display_text="what the user actually typed", + ) + + @pytest.mark.asyncio + async def test_a_tool_use_message_does_not_consume_the_arm(self, armed_hook, store): + await armed_hook.write_display_text( + _event( + { + "role": "user", + "content": [{"toolUse": {"toolUseId": "t1", "name": "x", "input": {}}}], + } + ) + ) + + store.assert_not_awaited() + + +class TestArming: + @pytest.mark.asyncio + async def test_unarmed_hook_writes_nothing(self, store): + hook = DisplayTextHook() + + await hook.write_display_text(_event(_user_message())) + + store.assert_not_awaited() + assert hook.wrote_this_turn is False + + @pytest.mark.asyncio + async def test_arming_with_no_text_disarms(self, store): + """A turn that sends the user's text verbatim β€” and every resume / + continuation, which sends no new user turn at all β€” passes None.""" + hook = DisplayTextHook() + hook.arm(session_id="s1", user_id="u1", message_index=4, display_text="orig") + hook.arm(session_id="s1", user_id="u1", message_index=6, display_text=None) + + await hook.write_display_text(_event(_user_message())) + + store.assert_not_awaited() + + @pytest.mark.asyncio + async def test_re_arming_replaces_the_previous_turns_state(self, store): + """The agent instance is cached across turns (#741/#751). + + A second turn must write ITS text at ITS index, never the first's. + """ + hook = DisplayTextHook() + hook.arm(session_id="s1", user_id="u1", message_index=4, display_text="first") + hook.arm(session_id="s1", user_id="u1", message_index=6, display_text="second") + + await hook.write_display_text(_event(_user_message())) + + store.assert_awaited_once_with( + session_id="s1", user_id="u1", message_id=6, display_text="second" + ) + + @pytest.mark.asyncio + async def test_re_arming_clears_the_written_flag(self, armed_hook, store): + """`wrote_this_turn` gates the coordinator's backstop, so a stale True + from last turn would suppress a write this turn genuinely needs.""" + await armed_hook.write_display_text(_event(_user_message())) + assert armed_hook.wrote_this_turn is True + + armed_hook.arm( + session_id="s1", user_id="u1", message_index=6, display_text="next turn" + ) + + assert armed_hook.wrote_this_turn is False + + +class TestFailSoft: + @pytest.mark.asyncio + async def test_a_storage_failure_never_reaches_the_turn(self, armed_hook): + with patch( + "apis.shared.sessions.metadata.store_user_display_text", + new_callable=AsyncMock, + side_effect=RuntimeError("dynamo down"), + ): + await armed_hook.write_display_text(_event(_user_message())) + + # Not marked written, so the coordinator's end-of-turn backstop still + # runs for a turn that completes. + assert armed_hook.wrote_this_turn is False + + @pytest.mark.asyncio + async def test_a_message_without_a_role_is_ignored(self, armed_hook, store): + await armed_hook.write_display_text(_event({"content": []})) + + store.assert_not_awaited() + + +class TestRegistration: + def test_registers_for_message_added(self): + hook = DisplayTextHook() + registry = MagicMock() + + hook.register_hooks(registry) + + registered = {call.args[0] for call in registry.add_callback.call_args_list} + assert MessageAddedEvent in registered diff --git a/backend/tests/agents/main_agent/streaming/test_display_text_write.py b/backend/tests/agents/main_agent/streaming/test_display_text_write.py new file mode 100644 index 00000000..0177d547 --- /dev/null +++ b/backend/tests/agents/main_agent/streaming/test_display_text_write.py @@ -0,0 +1,165 @@ +"""The coordinator's half of the displayText fix: arm early, back stop late. + +`displayText` is what the UI renders in place of a prompt the model saw but +the user never typed β€” RAG context, attachment guidance, an +``. It used to be written only here, at the end of a +successful turn, so a stopped or dropped turn left the augmented prompt as the +only renderable text. `DisplayTextHook` now writes it at append time instead. + +What stays the coordinator's job, and is tested here: + +1. **Arm the hook every turn, unconditionally β€” including to None.** The agent + instance is cached across turns (#741/#751); an arm left by a previous turn + would stamp its text onto this turn's message index. Same discipline as the + `turn_lease` stamp next to it. +2. **Back stop only what the hook didn't do.** A wrapper with no hook (voice, + tests) must keep the old end-of-turn write, and a hook whose write failed + must not silence it β€” but the normal path must not put twice. + +Driven through the real `stream_response`, like the steering-events suite. +""" + +from typing import Any, AsyncIterator, Dict, List, Optional +from unittest.mock import AsyncMock, patch + +import pytest + +from agents.main_agent.streaming.stream_coordinator import StreamCoordinator + + +class _FakeAgent: + def __init__(self) -> None: + self.messages = [{"role": "user", "content": [{"text": "hi"}]}] + + def stream_async(self, prompt: Any) -> AsyncIterator[Dict[str, Any]]: + async def _gen() -> AsyncIterator[Dict[str, Any]]: + return + yield # pragma: no cover - empty stream + + return _gen() + + +class _SessionManager: + def __init__(self) -> None: + self.cancelled = False + self.turn_lease = None + + async def update_after_turn(self, input_tokens, current_messages=None): + return None + + +class _RecordingHook: + """Stands in for DisplayTextHook β€” records arming, reports its result.""" + + def __init__(self, wrote: bool = False) -> None: + self.arms: List[dict] = [] + self._wrote = wrote + + def arm(self, **kwargs) -> None: + self.arms.append(kwargs) + + @property + def wrote_this_turn(self) -> bool: + return self._wrote + + +class _Wrapper: + def __init__(self, hook=None) -> None: + if hook is not None: + self.display_text_hook = hook + + +async def _run(wrapper=None, original_message: Optional[str] = None) -> None: + coordinator = StreamCoordinator() + async for _ in coordinator.stream_response( + agent=_FakeAgent(), + prompt="augmented prompt the model saw", + session_manager=_SessionManager(), + session_id="sess-1", + user_id="user-1", + main_agent_wrapper=wrapper, + original_message=original_message, + ): + pass + + +@pytest.fixture +def store(): + with patch( + "apis.shared.sessions.metadata.store_user_display_text", new_callable=AsyncMock + ) as mock: + yield mock + + +class TestArming: + @pytest.mark.asyncio + async def test_arms_the_hook_with_this_turns_text_and_index(self, store): + hook = _RecordingHook() + + await _run(_Wrapper(hook), original_message="what the user typed") + + assert hook.arms == [ + { + "session_id": "sess-1", + "user_id": "user-1", + "message_index": 0, + "display_text": "what the user typed", + } + ] + + @pytest.mark.asyncio + async def test_arms_to_none_when_the_prompt_was_not_modified(self, store): + """Unconditional arming is the point: a cached agent whose previous + turn was augmented must not write that turn's text against this one.""" + hook = _RecordingHook() + + await _run(_Wrapper(hook), original_message=None) + + assert hook.arms == [ + { + "session_id": "sess-1", + "user_id": "user-1", + "message_index": 0, + "display_text": None, + } + ] + + @pytest.mark.asyncio + async def test_a_wrapper_without_the_hook_is_not_an_error(self, store): + await _run(_Wrapper(), original_message="what the user typed") + await _run(None, original_message="what the user typed") + + +class TestBackstop: + @pytest.mark.asyncio + async def test_skipped_once_the_hook_has_written(self, store): + """The hook's write is the one that matters; repeating it at turn end + would put the same record twice on every augmented turn.""" + await _run(_Wrapper(_RecordingHook(wrote=True)), original_message="typed") + + store.assert_not_awaited() + + @pytest.mark.asyncio + async def test_runs_when_the_hook_write_failed(self, store): + """`wrote_this_turn` stays False on a storage failure, so a turn that + completes still gets its record.""" + await _run(_Wrapper(_RecordingHook(wrote=False)), original_message="typed") + + store.assert_awaited_once_with( + session_id="sess-1", user_id="user-1", message_id=0, display_text="typed" + ) + + @pytest.mark.asyncio + async def test_runs_for_a_wrapper_that_carries_no_hook(self, store): + """Voice and tests keep exactly the behaviour they had before.""" + await _run(_Wrapper(), original_message="typed") + + store.assert_awaited_once_with( + session_id="sess-1", user_id="user-1", message_id=0, display_text="typed" + ) + + @pytest.mark.asyncio + async def test_nothing_written_when_the_prompt_was_not_modified(self, store): + await _run(_Wrapper(), original_message=None) + + store.assert_not_awaited() diff --git a/backend/tests/apis/app_api/artifacts/test_artifact_share_inbox.py b/backend/tests/apis/app_api/artifacts/test_artifact_share_inbox.py index 47a350c0..87cbf5de 100644 --- a/backend/tests/apis/app_api/artifacts/test_artifact_share_inbox.py +++ b/backend/tests/apis/app_api/artifacts/test_artifact_share_inbox.py @@ -464,13 +464,29 @@ def test_inbox_404s_while_the_flag_is_off( env, monkeypatch: pytest.MonkeyPatch ) -> None: make_client, ddb = env - monkeypatch.delenv("ARTIFACT_SHARE_INBOX_ENABLED", raising=False) + monkeypatch.setenv("ARTIFACT_SHARE_INBOX_ENABLED", "false") _put_version(ddb) _share_with(make_client(), [FRIEND_EMAIL]) assert make_client(_friend()).get("/shared-artifacts").status_code == 404 +def test_inbox_is_on_when_the_flag_is_unset( + env, monkeypatch: pytest.MonkeyPatch +) -> None: + """The point of the default-on flip. + + A fork that never sets ``CDK_ARTIFACT_SHARE_INBOX_ENABLED`` should get + the finished feature, not lose it silently and have to discover a + variable to get it back.""" + make_client, ddb = env + monkeypatch.delenv("ARTIFACT_SHARE_INBOX_ENABLED", raising=False) + _put_version(ddb) + _share_with(make_client(), [FRIEND_EMAIL]) + + assert len(_inbox(make_client(_friend()))["artifacts"]) == 1 + + def test_fan_out_rows_are_written_while_the_flag_is_off( env, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -492,22 +508,25 @@ def test_fan_out_rows_are_written_while_the_flag_is_off( assert len(_inbox(make_client(_friend()))["artifacts"]) == 1 -def test_only_the_literal_true_enables_the_inbox( +def test_only_the_literal_false_disables_the_inbox( env, monkeypatch: pytest.MonkeyPatch ) -> None: - """Opt-in, not a kill switch. An unset GitHub Actions variable - forwards an empty string, which must resolve to off rather than - revealing the surface by accident.""" + """A kill switch, not opt-in β€” and the empty string is the case that + matters. An unset GitHub Actions variable forwards ``""``, which must + resolve to ON. Getting this backwards is how a default-on flag ships + silently disabled to every fork that never sets the variable.""" make_client, ddb = env _put_version(ddb) _share_with(make_client(), [FRIEND_EMAIL]) friend = make_client(_friend()) - for value in ("", " ", "false", "1", "yes", "TRUE!"): + for value in ("false", "FALSE", " False "): monkeypatch.setenv("ARTIFACT_SHARE_INBOX_ENABLED", value) assert friend.get("/shared-artifacts").status_code == 404, value - for value in ("true", "TRUE", " True "): + # Everything else β€” including the empty string, and including junk β€” + # leaves the feature on. A typo must not silently disable a surface. + for value in ("", " ", "true", "TRUE", " True ", "1", "yes", "FALSE!"): monkeypatch.setenv("ARTIFACT_SHARE_INBOX_ENABLED", value) assert friend.get("/shared-artifacts").status_code == 200, value diff --git a/backend/tests/routes/test_sessions.py b/backend/tests/routes/test_sessions.py index b16a3891..87896db5 100644 --- a/backend/tests/routes/test_sessions.py +++ b/backend/tests/routes/test_sessions.py @@ -1036,6 +1036,9 @@ def test_returns_204_and_records_navigated_away(self, app, make_user, authentica with patch( "apis.app_api.sessions.routes.set_interrupted_turn", recorder, + ), patch( + "apis.shared.sessions.session_lease.is_session_lease_held", + AsyncMock(return_value=True), ): resp = client.post( "/sessions/sess-001/interrupt", @@ -1050,6 +1053,76 @@ def test_returns_204_and_records_navigated_away(self, app, make_user, authentica source="client_signal", ) + def test_ignores_navigated_away_when_no_turn_is_in_flight( + self, app, make_user, authenticated_client + ): + """A departure can only interrupt a turn that is actually running. + + The SPA decides "is a turn in flight" from its own transport state, + and that has been wrong: a controller left behind after a completed + stream made every finished turn in the tab eligible, so a later + refresh marked complete answers as interrupted. The lease is the + server's own answer to the same question, and old tabs keep running + the old SPA long after a client fix ships β€” so assert it here. + """ + user = make_user() + client = authenticated_client(app, user) + + recorder = AsyncMock() + with patch( + "apis.app_api.sessions.routes.set_interrupted_turn", + recorder, + ), patch( + "apis.shared.sessions.session_lease.is_session_lease_held", + AsyncMock(return_value=False), + ): + resp = client.post( + "/sessions/sess-001/interrupt", + json={"reason": "navigated_away"}, + ) + + # Still 204: the client never waits on this and a dropped signal is + # not an error it can act on. + assert resp.status_code == 204 + recorder.assert_not_awaited() + + def test_user_stopped_is_not_gated_on_the_lease(self, app, make_user, authenticated_client): + """Stop is never withheld on a lease read. + + The button only exists while a response is streaming, and the same + request arms distributed cancellation β€” which must still reach a turn + whose lease read fails or fails open (`is_session_lease_held` reports + False for an unconfigured table and for any DynamoDB error). + """ + user = make_user() + client = authenticated_client(app, user) + + recorder = AsyncMock() + cancel = AsyncMock(return_value=True) + with patch( + "apis.app_api.sessions.routes.set_interrupted_turn", + recorder, + ), patch( + "apis.shared.sessions.session_lease.is_session_lease_held", + AsyncMock(return_value=False), + ), patch( + "apis.shared.sessions.session_lease.request_session_cancel", + cancel, + ): + resp = client.post( + "/sessions/sess-001/interrupt", + json={"reason": "user_stopped"}, + ) + + assert resp.status_code == 204 + recorder.assert_awaited_once_with( + "sess-001", + user.user_id, + reason="user_stopped", + source="client_signal", + ) + cancel.assert_awaited_once_with("sess-001", user.user_id) + def test_navigated_away_does_not_cancel_the_turn(self, app, make_user, authenticated_client): """Attribution, not instruction. @@ -1064,6 +1137,11 @@ def test_navigated_away_does_not_cancel_the_turn(self, app, make_user, authentic with patch( "apis.app_api.sessions.routes.set_interrupted_turn", AsyncMock(), + ), patch( + # Held, so the departure is genuinely mid-turn β€” otherwise this + # would assert nothing beyond the gate above. + "apis.shared.sessions.session_lease.is_session_lease_held", + AsyncMock(return_value=True), ), patch( "apis.shared.sessions.session_lease.request_session_cancel", cancel, diff --git a/backend/tests/test_backfill_artifact_user_index_keys.py b/backend/tests/test_backfill_artifact_user_index_keys.py new file mode 100644 index 00000000..6035f2ac --- /dev/null +++ b/backend/tests/test_backfill_artifact_user_index_keys.py @@ -0,0 +1,250 @@ +"""Tests for the UserArtifactsIndex key backfill. + +The failure this guards against is silent: `UserArtifactsIndex` is +sparse, so a HEAD row left without `GSI2PK`/`GSI2SK` is not stale in the +index β€” it is absent from it forever, and the library page simply stops +listing that artifact with no error anywhere. + +So the assertions that matter are about what the script *refuses* to do +(fabricate a sort key, touch `updated_at`, resurrect a deleted row) as +much as what it writes. +""" + +from __future__ import annotations + +import importlib.util +import pathlib + +import boto3 +import pytest +from moto import mock_aws + +REGION = "us-east-1" +TABLE = "test-user-artifacts" + +_SCRIPT = ( + pathlib.Path(__file__).resolve().parents[1] + / "scripts" + / "backfill_artifact_user_index_keys.py" +) +_spec = importlib.util.spec_from_file_location("backfill_user_index", _SCRIPT) +backfill_mod = importlib.util.module_from_spec(_spec) +assert _spec.loader is not None +_spec.loader.exec_module(backfill_mod) + + +@pytest.fixture() +def table(monkeypatch: pytest.MonkeyPatch): + monkeypatch.setenv("AWS_DEFAULT_REGION", REGION) + monkeypatch.setenv("AWS_ACCESS_KEY_ID", "testing") + monkeypatch.setenv("AWS_SECRET_ACCESS_KEY", "testing") + with mock_aws(): + ddb = boto3.resource("dynamodb", region_name=REGION) + ddb.create_table( + TableName=TABLE, + KeySchema=[ + {"AttributeName": "PK", "KeyType": "HASH"}, + {"AttributeName": "SK", "KeyType": "RANGE"}, + ], + AttributeDefinitions=[ + {"AttributeName": "PK", "AttributeType": "S"}, + {"AttributeName": "SK", "AttributeType": "S"}, + ], + BillingMode="PAY_PER_REQUEST", + ) + yield ddb.Table(TABLE) + + +def put_head( + table, + *, + user: str = "u1", + artifact: str = "a1", + updated_at: str | None = "2026-08-20T19:41:29.618536+00:00", + stamped: bool = False, + **extra, +) -> None: + item = { + "PK": f"USER#{user}", + "SK": f"ARTIFACT#{artifact}#HEAD", + "artifact_id": artifact, + "user_id": user, + "version": 1, + "title": "Deck", + "content_type": "text/html; charset=utf-8", + "session_id": "s1", + } + if updated_at is not None: + item["updated_at"] = updated_at + if stamped: + item["GSI2PK"] = f"USER#{user}" + item["GSI2SK"] = f"ARTIFACT#{updated_at}#{artifact}" + item.update(extra) + table.put_item(Item=item) + + +def put_version(table, *, user: str = "u1", artifact: str = "a1", version: int = 1): + table.put_item( + Item={ + "PK": f"USER#{user}", + "SK": f"ARTIFACT#{artifact}#V#{version:05d}", + "artifact_id": artifact, + "user_id": user, + "version": version, + "updated_at": "2026-08-20T19:41:29.618536+00:00", + } + ) + + +def row(table, user="u1", artifact="a1") -> dict: + return table.get_item( + Key={"PK": f"USER#{user}", "SK": f"ARTIFACT#{artifact}#HEAD"} + )["Item"] + + +# ------------------------------------------------------------------ +# What it writes +# ------------------------------------------------------------------ + + +def test_stamps_keys_matching_what_the_writer_would_have_written(table): + put_head(table, updated_at="2026-08-20T19:41:29.618536+00:00") + + stats = backfill_mod.backfill(table, apply=True) + + item = row(table) + assert item["GSI2PK"] == "USER#u1" + # Byte-for-byte the writer's format: ARTIFACT#{updated_at}#{aid}. + # A different shape here would order the index inconsistently with + # every row the writer stamps from now on. + assert item["GSI2SK"] == "ARTIFACT#2026-08-20T19:41:29.618536+00:00#a1" + assert stats["stamped"] == 1 + + +def test_never_touches_updated_at(table): + # updated_at is embedded in both GSI sort keys and is writer-owned. + # Bumping it here would reorder the library and desync GSI1SK. + put_head(table, updated_at="2026-08-20T19:41:29.618536+00:00") + + backfill_mod.backfill(table, apply=True) + + assert row(table)["updated_at"] == "2026-08-20T19:41:29.618536+00:00" + + +def test_leaves_version_rows_alone(table): + # The keys belong on HEAD only, so the index holds one row per + # artifact rather than one per version. + put_head(table) + put_version(table, version=1) + put_version(table, version=2) + + stats = backfill_mod.backfill(table, apply=True) + + assert stats["head_rows"] == 1 + for v in (1, 2): + item = table.get_item( + Key={"PK": "USER#u1", "SK": f"ARTIFACT#a1#V#{v:05d}"} + )["Item"] + assert "GSI2PK" not in item + + +def test_spans_every_user(table): + # Whole-table migration: the base table is partitioned by user, so a + # Query would only ever see one of them. + put_head(table, user="u1", artifact="a1") + put_head(table, user="u2", artifact="a2") + + stats = backfill_mod.backfill(table, apply=True) + + assert stats["stamped"] == 2 + assert row(table, "u2", "a2")["GSI2PK"] == "USER#u2" + + +# ------------------------------------------------------------------ +# What it refuses to do +# ------------------------------------------------------------------ + + +def test_refuses_to_fabricate_a_sort_key(table): + """A HEAD row with no `updated_at` is reported, never guessed at. + + GSI2SK is what the library orders by; inventing a timestamp would + sort that artifact wrongly for the rest of its life, invisibly.""" + put_head(table, updated_at=None) + + stats = backfill_mod.backfill(table, apply=True) + + assert stats["skipped"] == 1 + assert stats["stamped"] == 0 + assert "GSI2PK" not in row(table) + + +def test_dry_run_writes_nothing(table): + put_head(table) + + stats = backfill_mod.backfill(table, apply=False) + + assert stats["stamped"] == 1 # counted as *would* stamp + assert "GSI2PK" not in row(table) + + +def test_is_idempotent(table): + put_head(table) + + first = backfill_mod.backfill(table, apply=True) + second = backfill_mod.backfill(table, apply=True) + + assert first["stamped"] == 1 + assert second["stamped"] == 0 + assert second["already"] == 1 + + +def test_yields_to_a_row_the_writer_already_stamped(table): + # The writer's value wins; the script must not overwrite it. + put_head(table, stamped=True) + + stats = backfill_mod.backfill(table, apply=True) + + assert stats["already"] == 1 + assert stats["stamped"] == 0 + + +def test_does_not_resurrect_a_row_deleted_mid_run(table): + """The scan and the writes are not one transaction, so a row can be + deleted in between. `attribute_exists(SK)` makes that a no-op instead + of a resurrection β€” the same rule the writer's own write-backs use.""" + put_head(table) + rows = list(backfill_mod.iter_head_rows(table)) + assert len(rows) == 1 + + table.delete_item(Key={"PK": "USER#u1", "SK": "ARTIFACT#a1#HEAD"}) + + # Replay the write for the row we scanned before the delete. + plan = backfill_mod.plan_row(rows[0]) + assert plan is not None and "skip" not in plan + stats = backfill_mod.backfill(table, apply=True) + + assert stats["head_rows"] == 0 + assert ( + table.get_item( + Key={"PK": "USER#u1", "SK": "ARTIFACT#a1#HEAD"} + ).get("Item") + is None + ) + + +def test_reports_an_unexpected_partition_key(table): + put_head(table) + table.put_item( + Item={ + "PK": "SHARE#abc", + "SK": "ARTIFACT#weird#HEAD", + "artifact_id": "weird", + "updated_at": "2026-08-20T00:00:00+00:00", + } + ) + + stats = backfill_mod.backfill(table, apply=True) + + assert stats["stamped"] == 1 + assert stats["skipped"] == 1 diff --git a/backend/tests/test_backfill_false_interrupted_markers.py b/backend/tests/test_backfill_false_interrupted_markers.py new file mode 100644 index 00000000..6628abd2 --- /dev/null +++ b/backend/tests/test_backfill_false_interrupted_markers.py @@ -0,0 +1,194 @@ +"""Tests for the false interrupted-turn marker backfill. + +The risk this guards is asymmetric. Leaving a stale marker behind costs a +spurious "Response interrupted" chip and one false `` on +that session's next prompt; clearing a REAL one destroys the only record that +a turn was cut off, and with it the reload's offer to continue. So the +assertions that matter most are about what the script refuses to touch. + +The 900s threshold is the whole safety argument: a genuine interruption does +not always bump `lastMessageAt` (the "marker only, no synthetic write" branch +of `_persist_interruption`), so a modest gap is ambiguous β€” but no turn is +still running 15 minutes after its last message, because the stream times out +at 600s. +""" + +from __future__ import annotations + +import importlib.util +import pathlib + +import boto3 +import pytest +from moto import mock_aws + +REGION = "us-east-1" +TABLE = "test-sessions-metadata" + +_SCRIPT = ( + pathlib.Path(__file__).resolve().parents[1] + / "scripts" + / "backfill_false_interrupted_markers.py" +) +_spec = importlib.util.spec_from_file_location("backfill_interrupt_markers", _SCRIPT) +backfill_mod = importlib.util.module_from_spec(_spec) +assert _spec.loader is not None +_spec.loader.exec_module(backfill_mod) + + +@pytest.fixture() +def table(monkeypatch: pytest.MonkeyPatch): + monkeypatch.setenv("AWS_DEFAULT_REGION", REGION) + monkeypatch.setenv("AWS_ACCESS_KEY_ID", "testing") + monkeypatch.setenv("AWS_SECRET_ACCESS_KEY", "testing") + with mock_aws(): + ddb = boto3.resource("dynamodb", region_name=REGION) + ddb.create_table( + TableName=TABLE, + KeySchema=[ + {"AttributeName": "PK", "KeyType": "HASH"}, + {"AttributeName": "SK", "KeyType": "RANGE"}, + ], + AttributeDefinitions=[ + {"AttributeName": "PK", "AttributeType": "S"}, + {"AttributeName": "SK", "AttributeType": "S"}, + ], + BillingMode="PAY_PER_REQUEST", + ) + yield ddb.Table(TABLE) + + +def marked_row( + session_id: str, + *, + reason: str = "navigated_away", + last_message_at: str = "2026-09-01T12:00:00+00:00", + marked_at: str = "2026-09-01T13:00:00+00:00", +) -> dict: + return { + "PK": "USER#u1", + "SK": f"S#{session_id}", + "GSI_PK": f"SESSION#{session_id}", + "lastTurnInterrupted": True, + "lastTurnInterruptReason": reason, + "lastTurnInterruptedAt": marked_at, + "lastMessageAt": last_message_at, + "title": "keep me", + } + + +# --------------------------------------------------------------------------- +# Selection +# --------------------------------------------------------------------------- + + +def test_selects_navigated_away_marked_long_after_the_last_message(): + rows = [marked_row("s1")] # +1h + selected, _ = backfill_mod._select(rows, backfill_mod.DEFAULT_MIN_GAP_SECONDS) + assert [r["GSI_PK"] for r in selected] == ["SESSION#s1"] + + +@pytest.mark.parametrize("reason", ["user_stopped", "connection_lost", "unknown"]) +def test_never_selects_a_reason_this_bug_cannot_produce(reason: str): + """Only the client's `navigated_away` path had the stale-controller bug. + + `user_stopped` is the user's own attested intent and `connection_lost` + is the server's own backstop β€” clearing either would erase a real record. + """ + rows = [marked_row("s1", reason=reason)] + selected, skipped = backfill_mod._select(rows, backfill_mod.DEFAULT_MIN_GAP_SECONDS) + assert selected == [] + assert skipped["other_reason"] == 1 + + +def test_leaves_an_ambiguous_gap_alone(): + """A 10-minute gap is inside the 600s stream timeout. + + An interrupted continuation persists no assistant message, so + `lastMessageAt` stays at the previous turn and a real interruption can + show a positive gap. Below the threshold we cannot tell, so we don't act. + """ + rows = [marked_row("s1", marked_at="2026-09-01T12:10:00+00:00")] + selected, skipped = backfill_mod._select(rows, backfill_mod.DEFAULT_MIN_GAP_SECONDS) + assert selected == [] + assert skipped["gap_too_small"] == 1 + + +def test_leaves_a_marker_that_predates_the_last_message_alone(): + """The shape of a genuine mid-turn departure: the signal lands first, the + turn's partial is persisted after it.""" + rows = [ + marked_row( + "s1", + last_message_at="2026-09-01T12:00:30+00:00", + marked_at="2026-09-01T12:00:00+00:00", + ) + ] + selected, _ = backfill_mod._select(rows, backfill_mod.DEFAULT_MIN_GAP_SECONDS) + assert selected == [] + + +def test_leaves_unparseable_timestamps_alone(): + rows = [marked_row("s1", marked_at="not-a-date")] + selected, skipped = backfill_mod._select(rows, backfill_mod.DEFAULT_MIN_GAP_SECONDS) + assert selected == [] + assert skipped["unparseable"] == 1 + + +# --------------------------------------------------------------------------- +# Writes +# --------------------------------------------------------------------------- + + +def test_clear_removes_only_the_marker_attributes(table): + row = marked_row("s1") + table.put_item(Item=row) + + assert backfill_mod._clear(table, row) == "cleared" + + stored = table.get_item(Key={"PK": row["PK"], "SK": row["SK"]})["Item"] + for attr in backfill_mod.MARKER_ATTRS: + assert attr not in stored + # Everything else on the row survives β€” this is a targeted REMOVE, not a + # rewrite (a full put_item would drop attributes the script never read). + assert stored["title"] == "keep me" + assert stored["lastMessageAt"] == row["lastMessageAt"] + + +def test_clear_skips_a_row_re_marked_since_the_scan(table): + """A session that ran a new turn between scan and write may have been + legitimately re-marked. The write is conditional on the exact timestamp + the scan read, so it declines rather than clobbering.""" + row = marked_row("s1") + table.put_item(Item=row) + table.update_item( + Key={"PK": row["PK"], "SK": row["SK"]}, + UpdateExpression="SET lastTurnInterruptedAt = :ts", + ExpressionAttributeValues={":ts": "2026-09-02T09:00:00+00:00"}, + ) + + assert backfill_mod._clear(table, row) == "raced" + + stored = table.get_item(Key={"PK": row["PK"], "SK": row["SK"]})["Item"] + assert stored["lastTurnInterrupted"] is True + assert stored["lastTurnInterruptedAt"] == "2026-09-02T09:00:00+00:00" + + +def test_clear_skips_a_row_whose_reason_was_upgraded(table): + """`set_interrupted_turn` lets a stronger reason overwrite a weaker one. + If `user_stopped` landed after our scan, this is no longer our row.""" + row = marked_row("s1") + table.put_item(Item={**row, "lastTurnInterruptReason": "user_stopped"}) + + assert backfill_mod._clear(table, row) == "raced" + stored = table.get_item(Key={"PK": row["PK"], "SK": row["SK"]})["Item"] + assert stored["lastTurnInterruptReason"] == "user_stopped" + + +def test_clear_is_idempotent(table): + row = marked_row("s1") + table.put_item(Item=row) + + assert backfill_mod._clear(table, row) == "cleared" + # A second pass finds nothing to do rather than erroring. + assert backfill_mod._clear(table, row) == "raced" diff --git a/backend/uv.lock b/backend/uv.lock index 29576f9d..c995f68c 100644 --- a/backend/uv.lock +++ b/backend/uv.lock @@ -12,7 +12,7 @@ resolution-markers = [ [[package]] name = "agentcore-stack" -version = "1.18.0" +version = "1.19.0" source = { editable = "." } dependencies = [ { name = "aiofiles" }, diff --git a/docs-site/src/content/docs/features/artifacts.md b/docs-site/src/content/docs/features/artifacts.md index c54448a2..37f0164a 100644 --- a/docs-site/src/content/docs/features/artifacts.md +++ b/docs-site/src/content/docs/features/artifacts.md @@ -173,18 +173,22 @@ consumed by minting a render token, so it cannot be useful without artifacts being on. The "Shared with you" inbox has a flag of its own: `ARTIFACT_SHARE_INBOX_ENABLED` -(CDK: `CDK_ARTIFACT_SHARE_INBOX_ENABLED`). Unlike the kill-switch flags elsewhere -in the platform it is **default off and opt-in** β€” only the literal `"true"` -enables it β€” because the surface shipped ahead of the product decision about it. -While off, `GET /shared-artifacts` 404s and the SPA renders the library without -tabs. +(CDK: `CDK_ARTIFACT_SHARE_INBOX_ENABLED`). Like the other flags in the platform +it is **default on with a kill switch** β€” only the literal `"false"` disables it, +and an unset variable resolves to on. While off, `GET /shared-artifacts` 404s and +the SPA renders the library without tabs. + +It shipped default-off and opt-in in 1.18.0, because the surface landed ahead of +the product decision about it. That decision was made and the inbox went live, so +the default flipped: a deployment that never sets the variable should get the +finished feature rather than silently lose it. The flag gates the **read only**. Fan-out rows are written by every share regardless of it. That asymmetry is deliberate: if the writes were gated too, turning the flag on would reveal an inbox missing every share created while it was off β€” a wrong answer rather than an empty one, and one nobody could see was -wrong. Writing the rows regardless makes the flip complete and instant, with no -backfill to sequence. +wrong. Writing the rows regardless makes the toggle complete and instant in +either direction, with no backfill to sequence. ## Artifacts inside a shared conversation diff --git a/docs/specs/feature-announcements.md b/docs/specs/feature-announcements.md index bd199615..716fb9b9 100644 --- a/docs/specs/feature-announcements.md +++ b/docs/specs/feature-announcements.md @@ -88,6 +88,25 @@ An announcement carries `surfaces: list[Literal["panel", "banner", "modal"]]`. rendered by a sibling of `quota-warning-banner`. One line plus an optional CTA and a βœ•. + **Its text is a button that opens the announcement's own dialog.** The pill + renders no body, so without a way in, its only affordances are βœ• and an + optional external CTA β€” which trains people to dismiss unread, with What's + New (buried in the user menu) as the only other route to the content. It + opens the single-announcement dialog rather than the What's New list + deliberately: the pill named one thing, and handing back a list to search + through is a worse answer than the thing itself. The dialog owns the ack + from that point (`sourceSurface: "banner"`, so reach stats can tell a + banner-driven read from an interruption), and any of its exits retires the + pill durably β€” having read the body is a stronger signal of consumption + than clicking βœ• on a one-line strip. + + **A `requiresAck` announcement gets no βœ• on the pill.** Dismissal + suppression is rank-based and covers the banner *and* modal slots alike + (Β§D7), so a βœ• here would let a user retire a compliance notice from the + strip before the blocking modal ever fired β€” leaving no `acknowledged` + record anywhere. On those, the only way out is to open it and press the + button. + *Revised after PR-4 shipped.* It was first built as a full-bleed strip below the top nav. Two things moved it. Dismissing a strip that occupied layout reflowed the whole view, so it became an overlay; and what a banner @@ -128,6 +147,11 @@ bust. **Announcements never touch the model call path** (D12). | `dismissed` | User clicks βœ• or "Got it" | Suppresses banner and modal. Entry stays in the panel. | | `acknowledged` | User clicks the confirm button on a `requiresAck` modal | As `dismissed`, plus it is a durable record an admin can report on. | +The ack's `surface` records **where the gesture happened**, not which surface +owns the announcement: a dialog the user opened by clicking the banner's text +writes `banner` for every action it records, so reach stats can distinguish a +banner that earned a read from a modal the user never asked for. + They are ranked (`seen=1 < dismissed=2 < acknowledged=3`) and the stored rank **only ever increases**. The write is a conditional `UpdateExpression`: diff --git a/frontend/ai.client/package-lock.json b/frontend/ai.client/package-lock.json index 76fb3551..3fd84055 100644 --- a/frontend/ai.client/package-lock.json +++ b/frontend/ai.client/package-lock.json @@ -1,12 +1,12 @@ { "name": "ai.client", - "version": "1.18.0", + "version": "1.19.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "ai.client", - "version": "1.18.0", + "version": "1.19.0", "dependencies": { "@angular/cdk": "21.2.14", "@angular/common": "21.2.19", diff --git a/frontend/ai.client/package.json b/frontend/ai.client/package.json index 9aceb5a6..188c8a95 100644 --- a/frontend/ai.client/package.json +++ b/frontend/ai.client/package.json @@ -1,6 +1,6 @@ { "name": "ai.client", - "version": "1.18.0", + "version": "1.19.0", "scripts": { "ng": "ng", "prestart": "tsx scripts/branding/generate-brand-theme.ts && tsx scripts/branding/generate-surface-theme.ts && tsx scripts/branding/generate-surface-colors.ts && tsx scripts/branding/generate-favicons.ts", diff --git a/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.spec.ts b/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.spec.ts index 5c5872e0..254faeb1 100644 --- a/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.spec.ts +++ b/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.spec.ts @@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { TestBed } from '@angular/core/testing'; import { signal } from '@angular/core'; import { AnnouncementsService } from '../../services/announcements/announcements.service'; +import { AnnouncementModalService } from '../../services/announcements/announcement-modal.service'; import { Announcement } from '../../services/announcements/announcement.model'; import { AnnouncementBannerComponent } from './announcement-banner.component'; @@ -28,16 +29,19 @@ function makeAnnouncement(overrides: Partial = {}): Announcement { describe('AnnouncementBannerComponent', () => { let bannerItem: ReturnType>; let ack: ReturnType; + let openFor: ReturnType; beforeEach(() => { TestBed.resetTestingModule(); bannerItem = signal(null); ack = vi.fn(async () => true); + openFor = vi.fn(); // DI-token override rather than vi.mock, per house convention. TestBed.configureTestingModule({ providers: [ { provide: AnnouncementsService, useValue: { bannerItem, ack } }, + { provide: AnnouncementModalService, useValue: { openFor } }, ], }); }); @@ -127,12 +131,11 @@ describe('AnnouncementBannerComponent', () => { const fixture = create(); ack.mockClear(); + // By label, not "the first button" β€” the text is a button too now. const dismiss = (fixture.nativeElement as HTMLElement).querySelector( - 'button', + 'button[aria-label="Dismiss announcement: Skills are here"]', ) as HTMLButtonElement; - expect(dismiss.getAttribute('aria-label')).toBe( - 'Dismiss announcement: Skills are here', - ); + expect(dismiss).not.toBeNull(); dismiss.click(); expect(ack).toHaveBeenCalledWith('a1', 'dismissed', 'banner'); @@ -173,6 +176,7 @@ describe('AnnouncementBannerComponent', () => { TestBed.configureTestingModule({ providers: [ { provide: AnnouncementsService, useValue: { bannerItem, ack } }, + { provide: AnnouncementModalService, useValue: { openFor } }, ], }); bannerItem.set( @@ -184,6 +188,103 @@ describe('AnnouncementBannerComponent', () => { expect(link.getAttribute('rel')).toBe('noopener noreferrer'); }); + describe('the text opens the full announcement', () => { + function readMore(fixture: ReturnType) { + // By `title`, which carries the untruncated headline β€” the βœ• is the one + // with an aria-label. + return (fixture.nativeElement as HTMLElement).querySelector( + 'button[title="Skills are here"]', + ) as HTMLButtonElement | null; + } + + it('hands the announcement to the modal service, attributed to the banner', () => { + // The pill renders no body, so without this the only affordances on it + // are βœ• and an optional CTA β€” which trains people to dismiss unread. + bannerItem.set(makeAnnouncement()); + const fixture = create(); + + readMore(fixture)!.click(); + + expect(openFor).toHaveBeenCalledWith( + expect.objectContaining({ announcement_id: 'a1' }), + 'banner', + ); + }); + + it('writes no ack of its own β€” the dialog owns that', () => { + // Acking `dismissed` here as well would be a second, racing write for + // the same gesture, and would retire the pill before the user has read + // a word of the body. + bannerItem.set(makeAnnouncement()); + const fixture = create(); + ack.mockClear(); + + readMore(fixture)!.click(); + + expect(ack).not.toHaveBeenCalled(); + }); + + it('keeps the visible text inside the accessible name (WCAG 2.5.3)', () => { + // `bannerText()` may be the summary, so an aria-label built from the + // title would leave a voice-control user saying a phrase that is not + // the button's name. The visible words have to survive. + bannerItem.set(makeAnnouncement({ summary: 'Short version' })); + const fixture = create(); + const button = readMore(fixture)!; + + expect(button.getAttribute('aria-label')).toBeNull(); + expect(button.textContent).toContain('Short version'); + expect(button.textContent).toContain('Read more'); + // The untruncated headline is still reachable on hover. + expect(button.getAttribute('title')).toBe('Skills are here'); + }); + }); + + describe('a requiresAck announcement cannot be retired from the strip', () => { + it('offers no βœ•', () => { + // `dismissed` and `acknowledged` both sit at or above SUPPRESSING_RANK, + // and suppression covers the banner AND modal slots β€” so a βœ• here would + // let a user kill a compliance notice before the blocking modal ever + // fired, leaving no `acknowledged` record anywhere. + bannerItem.set(makeAnnouncement({ requires_ack: true })); + const fixture = create(); + + expect( + (fixture.nativeElement as HTMLElement).querySelector( + 'button[aria-label^="Dismiss announcement"]', + ), + ).toBeNull(); + }); + + it('still opens on click β€” that is the only way out', () => { + bannerItem.set(makeAnnouncement({ requires_ack: true })); + const fixture = create(); + + ( + (fixture.nativeElement as HTMLElement).querySelector( + 'button[title="Skills are here"]', + ) as HTMLButtonElement + ).click(); + + expect(openFor).toHaveBeenCalledWith( + expect.objectContaining({ requires_ack: true }), + 'banner', + ); + }); + + it('writes no `dismissed` even if onDismiss is reached some other way', () => { + bannerItem.set(makeAnnouncement({ requires_ack: true })); + const fixture = create(); + ack.mockClear(); + + ( + fixture.componentInstance as unknown as { onDismiss(): void } + ).onDismiss(); + + expect(ack).not.toHaveBeenCalled(); + }); + }); + describe('overlays rather than occupying space', () => { it('positions the host absolutely, so dismissing it cannot reflow the page', () => { // The regression: the banner used to be a flex child of the shell's diff --git a/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.ts b/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.ts index e2882259..3620d110 100644 --- a/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.ts +++ b/frontend/ai.client/src/app/components/announcement-banner/announcement-banner.component.ts @@ -14,6 +14,7 @@ import { heroXMark, } from '@ng-icons/heroicons/outline'; import { AnnouncementsService } from '../../services/announcements/announcements.service'; +import { AnnouncementModalService } from '../../services/announcements/announcement-modal.service'; import { Announcement, AnnouncementSeverity, @@ -80,6 +81,22 @@ import { * Body markdown is deliberately *not* rendered here: this surface is one line. * The full body lives in What's New, which is why `panel` is forced onto every * announcement server-side. + * + * **The text is a button, and clicking it opens the full announcement.** + * Without it the only affordances on the pill are βœ• and an optional CTA, which + * trains people to dismiss unread β€” and What's New, the only other way to the + * body, is buried in the user menu. It opens the single-announcement dialog + * rather than the What's New list on purpose: the pill named one thing, so + * handing back a list to search is a worse answer than the thing itself. From + * there the dialog owns the ack, and any of its exits retires the pill + * durably (see `onOpenDetail`). + * + * **A `requiresAck` announcement gets no βœ• here.** `dismissed` and + * `acknowledged` are both at or above `SUPPRESSING_RANK`, and suppression + * applies to the banner *and* modal slots alike β€” so with a βœ• on the strip, a + * user could retire a compliance notice from the pill and the blocking modal + * would never fire, leaving no `acknowledged` record anywhere. On those, the + * only way out is to open it and press the button. */ @Component({ selector: 'app-announcement-banner', @@ -117,9 +134,19 @@ import { aria-hidden="true" /> -

- {{ bannerText() }} -

+ @if (item.cta_url && item.cta_label) { } - + @if (!item.requires_ack) { + + } } `, }) export class AnnouncementBannerComponent { private readonly announcements = inject(AnnouncementsService); + private readonly modals = inject(AnnouncementModalService); /** * Which side of the composer to take. `'above'` suits a bottom-pinned @@ -221,7 +251,24 @@ export class AnnouncementBannerComponent { protected onDismiss(): void { const item = this.announcement(); - if (!item) return; + if (!item || item.requires_ack) return; void this.announcements.ack(item.announcement_id, 'dismissed', 'banner'); } + + /** + * Open the full announcement, attributing whatever the user does there to + * the banner. + * + * The dialog owns the ack from here: any of its exits writes `dismissed` + * (or `acknowledged`), which outranks the `seen` this strip already wrote + * and retires the pill on every device. That is the intent β€” having read + * the body is a stronger signal of consumption than clicking βœ• on a + * one-line strip, and leaving the pill up afterwards would just ask the + * user to dismiss something they have already dealt with. + */ + protected onOpenDetail(): void { + const item = this.announcement(); + if (!item) return; + this.modals.openFor(item, 'banner'); + } } diff --git a/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.spec.ts b/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.spec.ts index 00db445b..3c1e562f 100644 --- a/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.spec.ts +++ b/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.spec.ts @@ -3,7 +3,10 @@ import { TestBed } from '@angular/core/testing'; import { DIALOG_DATA, DialogRef } from '@angular/cdk/dialog'; import { provideMarkdown } from 'ngx-markdown'; import { AnnouncementsService } from '../../services/announcements/announcements.service'; -import { Announcement } from '../../services/announcements/announcement.model'; +import { + Announcement, + AnnouncementSurface, +} from '../../services/announcements/announcement.model'; import { AnnouncementModalComponent, AnnouncementModalData, @@ -33,7 +36,10 @@ describe('AnnouncementModalComponent', () => { let ack: ReturnType; let close: ReturnType; - function setup(announcement: Announcement) { + function setup( + announcement: Announcement, + sourceSurface?: AnnouncementSurface, + ) { TestBed.resetTestingModule(); ack = vi.fn(async () => true); close = vi.fn(); @@ -45,7 +51,10 @@ describe('AnnouncementModalComponent', () => { { provide: DialogRef, useValue: { close, closed: { subscribe: vi.fn() } } }, { provide: DIALOG_DATA, - useValue: { announcement } satisfies AnnouncementModalData, + useValue: { + announcement, + sourceSurface, + } satisfies AnnouncementModalData, }, ], }); @@ -172,6 +181,33 @@ describe('AnnouncementModalComponent', () => { expect(link.getAttribute('rel')).toBe('noopener noreferrer'); }); + describe('ack attribution', () => { + it('defaults to the `modal` surface β€” the Β§D8 interruption', () => { + setup(makeAnnouncement()); + expect(ack).toHaveBeenCalledWith('a1', 'seen', 'modal'); + }); + + it('attributes every ack to the surface the user came from', () => { + // Opened by clicking the banner text. The ack row's `surface` is the + // only record of what drove the dismissal, so it must say `banner` β€” + // otherwise banner engagement is indistinguishable from an interruption + // the user never asked for. + const fixture = setup(makeAnnouncement(), 'banner'); + expect(ack).toHaveBeenCalledWith('a1', 'seen', 'banner'); + + ack.mockClear(); + confirmButton(fixture).click(); + expect(ack).toHaveBeenCalledWith('a1', 'dismissed', 'banner'); + }); + + it('carries the surface through an `acknowledged` too', () => { + const fixture = setup(makeAnnouncement({ requires_ack: true }), 'banner'); + ack.mockClear(); + confirmButton(fixture).click(); + expect(ack).toHaveBeenCalledWith('a1', 'acknowledged', 'banner'); + }); + }); + it('is a labelled modal dialog', () => { const fixture = setup(makeAnnouncement()); const panel = el(fixture).querySelector('[role="dialog"]')!; diff --git a/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.ts b/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.ts index b93ab17c..8202bc39 100644 --- a/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.ts +++ b/frontend/ai.client/src/app/components/announcement-modal/announcement-modal.component.ts @@ -10,10 +10,24 @@ import { NgIcon, provideIcons } from '@ng-icons/core'; import { heroXMark } from '@ng-icons/heroicons/outline'; import { DialogDismissDirective } from '../dialog/dialog-dismiss.directive'; import { AnnouncementsService } from '../../services/announcements/announcements.service'; -import { Announcement } from '../../services/announcements/announcement.model'; +import { + Announcement, + AnnouncementSurface, +} from '../../services/announcements/announcement.model'; export interface AnnouncementModalData { announcement: Announcement; + /** + * Which surface the user came from, for ack attribution. + * + * Defaults to `'modal'` β€” the Β§D8 interruption, where the dialog *is* the + * surface. The banner passes `'banner'` when the user clicks its text to + * read the body, because the ack row's `surface` is the only record of what + * actually drove the dismissal, and "the banner earned a read" is the one + * number this interaction exists to produce. The funnel counters key on + * action alone, so attribution never distorts them. + */ + sourceSurface?: AnnouncementSurface; } /** @@ -147,6 +161,9 @@ export class AnnouncementModalComponent { protected readonly titleId = `announcement-modal-title-${crypto.randomUUID()}`; protected readonly announcement = this.data.announcement; + /** Where every ack from this dialog is attributed. See `AnnouncementModalData`. */ + private readonly surface: AnnouncementSurface = this.data.sourceSurface ?? 'modal'; + protected readonly requiresAck = computed( () => this.announcement.requires_ack, ); @@ -168,7 +185,7 @@ export class AnnouncementModalComponent { void this.announcements.ack( this.announcement.announcement_id, 'seen', - 'modal', + this.surface, ); } @@ -177,7 +194,7 @@ export class AnnouncementModalComponent { void this.announcements.ack( this.announcement.announcement_id, this.requiresAck() ? 'acknowledged' : 'dismissed', - 'modal', + this.surface, ); this.dialogRef.close(); } @@ -187,7 +204,7 @@ export class AnnouncementModalComponent { void this.announcements.ack( this.announcement.announcement_id, 'dismissed', - 'modal', + this.surface, ); this.dialogRef.close(); } diff --git a/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.spec.ts b/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.spec.ts index f9db71f7..ce1dcb96 100644 --- a/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.spec.ts +++ b/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.spec.ts @@ -31,7 +31,7 @@ describe('SessionList', () => { mergedSessionsResource: signal({ sessions: [mockSession], nextToken: null }), currentSession: signal(mockSession), deleteSession: vi.fn().mockResolvedValue(undefined), - sessionsResource: { value: vi.fn().mockReturnValue(null), error: vi.fn().mockReturnValue(null), isPending: vi.fn().mockReturnValue(false) }, + sessionsResource: { value: vi.fn().mockReturnValue({ sessions: [mockSession], nextToken: null }), error: vi.fn().mockReturnValue(null), isPending: vi.fn().mockReturnValue(false) }, isLocallyRead: vi.fn().mockReturnValue(false), markSessionRead: vi.fn().mockResolvedValue(undefined), markSessionUnread: vi.fn().mockResolvedValue(undefined), @@ -62,6 +62,53 @@ describe('SessionList', () => { return TestBed.runInInjectionContext(() => new SessionList()); } + describe('isLoading', () => { + it('stays loading while the resource has not produced a response', async () => { + // Cold start: the loader short-circuits to `null` because sessions + // loading is not enabled until the BFF bootstrap resolves β€” and + // `reload()` keeps that `null` for the whole real fetch. With nothing + // cached to draw, that must read as loading (skeleton), never as + // "no conversations". + mockSessionService.mergedSessionsResource.set({ sessions: [], nextToken: null }); + mockSessionService.sessionsResource.value.mockReturnValue(null); + const component = await createComponent(); + + expect(component.isLoading()).toBe(true); + + // Before the first load resolves at all. + mockSessionService.sessionsResource.value.mockReturnValue(undefined); + expect(component.isLoading()).toBe(true); + }); + + it('is not loading once the API answers, even with zero sessions', async () => { + mockSessionService.mergedSessionsResource.set({ sessions: [], nextToken: null }); + mockSessionService.sessionsResource.value.mockReturnValue({ sessions: [], nextToken: null }); + const component = await createComponent(); + + // A real empty response is the empty state, not a skeleton. + expect(component.isLoading()).toBe(false); + }); + + it('renders locally cached sessions instead of a skeleton', async () => { + mockSessionService.sessionsResource.value.mockReturnValue(null); + const component = await createComponent(); + + expect(component.isLoading()).toBe(false); + }); + + it('defers to the error state without reading the resource value', async () => { + mockSessionService.mergedSessionsResource.set({ sessions: [], nextToken: null }); + mockSessionService.sessionsResource.error.mockReturnValue(new Error('boom')); + // Angular's resource throws from `value()` when the load errored. + mockSessionService.sessionsResource.value.mockImplementation(() => { + throw new Error('should not be read'); + }); + const component = await createComponent(); + + expect(component.isLoading()).toBe(false); + }); + }); + it('should compute sessions from merged resource', async () => { const component = await createComponent(); expect(component.sessions()).toEqual([mockSession]); diff --git a/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.ts b/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.ts index ba98be10..0f4a7d8d 100644 --- a/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.ts +++ b/frontend/ai.client/src/app/components/sidenav/components/session-list/session-list.ts @@ -132,11 +132,32 @@ export class SessionList { }); /** - * Computed signal for loading state. + * Computed signal for loading state β€” i.e. "we have nothing to draw yet". + * + * `value()` alone is not enough. The loader short-circuits to `null` while + * `sessionsRequest` is still false, and that is the ordinary cold-start path: + * `SessionService` is constructed during the APP_INITIALIZER pass (via + * `AnnouncementModalService` -> `MessageMapService`), and Angular runs every + * initializer synchronously before awaiting any of them β€” so the BFF + * `bootstrap()` promise is still in flight and `isAuthenticated()` is false. + * The eager-fetch branch in the constructor is skipped, the resource resolves + * `null`, and the auth effect only enables loading afterwards. `reload()` + * keeps the previous value, so `null` survives the entire real fetch: testing + * `=== undefined` reported "loaded, no sessions" and the sidebar rendered the + * "No Chats Yet" empty state instead of the skeleton. + * + * So: no API response yet (`undefined` before the first load resolves, `null` + * while it is short-circuited) means loading. An empty `sessions` array is a + * real response and must fall through to the empty state. + * + * `error()` is read first β€” reading `value()` on an errored resource throws. */ readonly isLoading = computed(() => { - const value = this.sessionsResource.value(); - return value === undefined; + if (this.sessionsResource.error()) return false; + if (this.sessionsResource.value() != null) return false; + // Locally created sessions can exist before the API answers; draw those + // rather than covering them with a skeleton. + return this.groupedSessions().length === 0; }); /** diff --git a/frontend/ai.client/src/app/services/announcements/announcement-modal.service.spec.ts b/frontend/ai.client/src/app/services/announcements/announcement-modal.service.spec.ts index 0784bf83..da958a6e 100644 --- a/frontend/ai.client/src/app/services/announcements/announcement-modal.service.spec.ts +++ b/frontend/ai.client/src/app/services/announcements/announcement-modal.service.spec.ts @@ -233,6 +233,64 @@ describe('AnnouncementModalService (Β§D8 turn-safety gate)', () => { expect(open).not.toHaveBeenCalled(); }); + describe('openFor β€” the user asked for it', () => { + it('opens even when every Β§D8 gate would refuse an interruption', () => { + // A click is not an interruption. The gate exists to stop us throwing a + // dialog at someone mid-thought; here the user is the one asking. + isLoadingSession.set('session-1'); + toolApprovalPending.set(true); + focusComposer('half a thought'); + const service = start(); + + service.openFor(makeAnnouncement(), 'banner'); + + expect(open).toHaveBeenCalledTimes(1); + const [, config] = open.mock.calls[0]; + expect(config.data.announcement.announcement_id).toBe('a1'); + expect(config.data.sourceSurface).toBe('banner'); + }); + + it('defaults the surface to `modal` when no source is given', () => { + const service = start(); + service.openFor(makeAnnouncement()); + expect(open.mock.calls[0][1].data.sourceSurface).toBe('modal'); + }); + + it('keeps disableClose on a requiresAck announcement', () => { + // Opening it yourself is not a way around the acknowledgement. + const service = start(); + service.openFor(makeAnnouncement({ requires_ack: true }), 'banner'); + expect(open.mock.calls[0][1].disableClose).toBe(true); + }); + + it('refuses to stack a second dialog on an open one', () => { + const service = start(); + service.openFor(makeAnnouncement(), 'banner'); + service.openFor(makeAnnouncement({ announcement_id: 'a2' }), 'banner'); + expect(open).toHaveBeenCalledTimes(1); + }); + + it('stops the Β§D8 effect re-opening what the user already read', () => { + // Same announcement in both slots β€” without marking it shown, the user + // would read it from the banner and then be interrupted by it on the + // next navigation. + const service = start(); + service.openFor(makeAnnouncement(), 'banner'); + expect(open).toHaveBeenCalledTimes(1); + + // Let the dialog close, so `openRef` is not what is holding it back. + const onClosed = open.mock.results[0].value.closed.subscribe.mock + .calls[0][0] as () => void; + onClosed(); + + modalItem.set(makeAnnouncement()); + TestBed.tick(); + navigate(); + + expect(open).toHaveBeenCalledTimes(1); + }); + }); + it('opens on the next settled navigation after a failed gate', () => { isLoadingSession.set('session-1'); start(); diff --git a/frontend/ai.client/src/app/services/announcements/announcement-modal.service.ts b/frontend/ai.client/src/app/services/announcements/announcement-modal.service.ts index 082c3eb7..f0365c5f 100644 --- a/frontend/ai.client/src/app/services/announcements/announcement-modal.service.ts +++ b/frontend/ai.client/src/app/services/announcements/announcement-modal.service.ts @@ -9,6 +9,7 @@ import { AnnouncementModalComponent, AnnouncementModalData, } from '../../components/announcement-modal/announcement-modal.component'; +import { Announcement, AnnouncementSurface } from './announcement.model'; import { SessionService } from '../../auth/session.service'; import { MessageMapService } from '../../session/services/session/message-map.service'; import { ToolApprovalService } from '../tool-approval/tool-approval.service'; @@ -95,25 +96,57 @@ export class AnnouncementModalService { untracked(() => { if (this.shown.has(item.announcement_id)) return; if (!this.canInterrupt()) return; - - this.shown.add(item.announcement_id); - this.openRef = this.dialog.open( - AnnouncementModalComponent, - { - data: { announcement: item }, - hasBackdrop: false, // the dialog component owns its own backdrop - // The only exit from a `requiresAck` announcement is its button. - disableClose: item.requires_ack, - panelClass: 'announcement-modal', - }, - ); - this.openRef.closed.subscribe(() => { - this.openRef = null; - }); + this.open(item, 'modal'); }); }); } + /** + * Open the detail dialog for one announcement **because the user asked**. + * + * The banner calls this when its text is clicked: the pill is one line and + * renders no body, so without a way in, the only affordances on it are βœ• and + * an optional CTA β€” which is how you train people to dismiss unread. + * + * Two things it deliberately does *not* do. It does not consult + * `canInterrupt()`: that gate exists to stop us throwing a dialog at someone + * mid-thought, and a click is not an interruption β€” the user is the one + * asking. It *does* respect `openRef`, because two stacked dialogs is a bug + * whoever opened them. + * + * It also marks the announcement `shown`, so the Β§D8 effect cannot re-open + * the same item as an interruption after the user has already read it here. + */ + openFor( + announcement: Announcement, + sourceSurface: AnnouncementSurface = 'modal', + ): void { + if (this.openRef !== null) return; + this.open(announcement, sourceSurface); + } + + /** The single place a dialog is constructed, so `openRef` cannot drift. */ + private open( + announcement: Announcement, + sourceSurface: AnnouncementSurface, + ): void { + this.shown.add(announcement.announcement_id); + this.openRef = this.dialog.open( + AnnouncementModalComponent, + { + data: { announcement, sourceSurface }, + hasBackdrop: false, // the dialog component owns its own backdrop + // The only exit from a `requiresAck` announcement is its button β€” + // including when the user opened it themselves from the banner. + disableClose: announcement.requires_ack, + panelClass: 'announcement-modal', + }, + ); + this.openRef.closed.subscribe(() => { + this.openRef = null; + }); + } + /** * The Β§D8 gate. Every read here is a snapshot β€” see the class comment for * why this must not be reactive. diff --git a/frontend/ai.client/src/app/session/services/chat/chat-http.service.spec.ts b/frontend/ai.client/src/app/session/services/chat/chat-http.service.spec.ts index d3c2def6..23ec7c97 100644 --- a/frontend/ai.client/src/app/session/services/chat/chat-http.service.spec.ts +++ b/frontend/ai.client/src/app/session/services/chat/chat-http.service.spec.ts @@ -29,9 +29,9 @@ describe('ChatHttpService', () => { // and lets cookie auth ride along with the request rather than // attaching a Bearer manually. { provide: BffSessionService, useValue: { csrfHeaders: vi.fn().mockReturnValue({}), handleUnauthorized: vi.fn() } }, - { provide: SessionService, useValue: { currentSession: signal({ sessionId: 's1' }), updateSessionTitleInCache: vi.fn(), getSessionMetadata: vi.fn().mockResolvedValue({}) } }, + { provide: SessionService, useValue: { currentSession: signal({ sessionId: 's1' }), updateSessionTitleInCache: vi.fn(), getSessionMetadata: vi.fn().mockResolvedValue({}), isNewSession: vi.fn().mockReturnValue(false) } }, { provide: StreamParserService, useValue: { getCurrentStreamId: vi.fn().mockReturnValue('stream-1'), parseEventSourceMessage: vi.fn() } }, - { provide: ChatStateService, useValue: { abortRequest: vi.fn(), setChatLoading: vi.fn(), setLastTurnInterrupted: vi.fn(), seedSessionAggregates: vi.fn(), createAbortController: vi.fn().mockReturnValue(new AbortController()), streamingSessionIds: vi.fn().mockReturnValue([]) } }, + { provide: ChatStateService, useValue: { abortRequest: vi.fn(), setChatLoading: vi.fn(), setLastTurnInterrupted: vi.fn(), seedSessionAggregates: vi.fn(), createAbortController: vi.fn().mockReturnValue(new AbortController()), releaseAbortController: vi.fn(), streamingSessionIds: vi.fn().mockReturnValue([]) } }, { provide: MessageMapService, useValue: { endStreaming: vi.fn() } }, { provide: ErrorService, useValue: { handleHttpError: vi.fn(), addError: vi.fn() } }, ], @@ -145,6 +145,28 @@ describe('ChatHttpService', () => { vi.restoreAllMocks(); }); + it('releases the session controller when a stream finishes normally', async () => { + // Without this the controller outlives its stream for the life of the + // tab, so `streamingSessionIds()` keeps reporting a finished turn and the + // next page-hide attributes it to `navigated_away` β€” a "Response + // interrupted" chip on a complete answer. + const controller = new AbortController(); + chatStateService.createAbortController.mockReturnValue(controller); + vi.spyOn(globalThis, 'fetch').mockResolvedValue( + new Response('event: done\ndata: {}\n\n', { + status: 200, + headers: { 'content-type': 'text/event-stream' }, + }), + ); + + await service.sendChatRequest({ session_id: 's1', message: 'hi' }); + + expect(chatStateService.releaseAbortController).toHaveBeenCalledWith('s1', controller); + // Released, not aborted β€” the turn completed on its own. + expect(controller.signal.aborted).toBe(false); + vi.restoreAllMocks(); + }); + it('surfaces a soft "Already responding" notice (not a hard error) on a 409 single-flight rejection', async () => { // The inference-api single-flight guard rejects a duplicate turn while the // prior one is still streaming server-side; the BFF relays it as 409. diff --git a/frontend/ai.client/src/app/session/services/chat/chat-http.service.ts b/frontend/ai.client/src/app/session/services/chat/chat-http.service.ts index 9c31cf91..c1333c2e 100644 --- a/frontend/ai.client/src/app/session/services/chat/chat-http.service.ts +++ b/frontend/ai.client/src/app/session/services/chat/chat-http.service.ts @@ -134,6 +134,11 @@ export class ChatHttpService { if (!isCurrentStream()) return; this.messageMapService.endStreaming(sessionId); this.chatStateService.setChatLoading(sessionId, false); + // Release the transport handle too. `streamingSessionIds()` reads it + // to decide which turns a page departure interrupted, so a controller + // left behind here makes every completed turn in this tab eligible for + // a `navigated_away` marker on the next refresh / tab close. + this.chatStateService.releaseAbortController(sessionId, abortController); }; try { diff --git a/frontend/ai.client/src/app/session/services/chat/chat-state.service.spec.ts b/frontend/ai.client/src/app/session/services/chat/chat-state.service.spec.ts index 5380c061..cce75972 100644 --- a/frontend/ai.client/src/app/session/services/chat/chat-state.service.spec.ts +++ b/frontend/ai.client/src/app/session/services/chat/chat-state.service.spec.ts @@ -205,5 +205,39 @@ describe('ChatStateService', () => { it('abortRequest is a no-op for sessions without an in-flight request', () => { expect(() => service.abortRequest('nope')).not.toThrow(); }); + + it('drops a session from streamingSessionIds once its controller is released', () => { + // The bug this guards: a completed stream used to leave its controller + // behind, so the session looked in-flight for the life of the tab and + // the next page-hide marked its finished turn `navigated_away`. + const a = service.createAbortController('a'); + service.createAbortController('b'); + + expect(service.streamingSessionIds().sort()).toEqual(['a', 'b']); + + service.releaseAbortController('a', a); + + expect(service.streamingSessionIds()).toEqual(['b']); + // Released, not aborted β€” the stream finished on its own. + expect(a.signal.aborted).toBe(false); + }); + + it('releaseAbortController ignores a controller the session no longer owns', () => { + // A superseded stream's late teardown must not clear the controller of + // the stream that replaced it. + const first = service.createAbortController('a'); + const second = service.createAbortController('a'); + + service.releaseAbortController('a', first); + + expect(service.streamingSessionIds()).toEqual(['a']); + expect(second.signal.aborted).toBe(false); + }); + + it('releaseAbortController is a no-op for an unknown session', () => { + expect(() => + service.releaseAbortController('nope', new AbortController()), + ).not.toThrow(); + }); }); }); diff --git a/frontend/ai.client/src/app/session/services/chat/chat-state.service.ts b/frontend/ai.client/src/app/session/services/chat/chat-state.service.ts index df3df963..6f81d303 100644 --- a/frontend/ai.client/src/app/session/services/chat/chat-state.service.ts +++ b/frontend/ai.client/src/app/session/services/chat/chat-state.service.ts @@ -234,12 +234,35 @@ export class ChatStateService { return controller; } + /** + * Release a session's controller once its stream has finished. + * + * The counterpart to `createAbortController`, and what makes + * `streamingSessionIds()` honest: without it a controller outlives its + * stream for the life of the tab, so every completed turn keeps looking + * in-flight and the page-hide attribution below marks turns that + * finished minutes (or days) earlier as `navigated_away`. That is a + * false "Response interrupted" chip on a complete answer, and a false + * interruption note prepended to the session's next prompt. + * + * Identity-checked: a superseded stream's late teardown must not clear + * the controller of the stream that replaced it (`createAbortController` + * installs the replacement before the old stream's abort unwinds). + */ + releaseAbortController(sessionId: string, controller: AbortController): void { + const state = this.states().get(sessionId); + if (state && state.abortController === controller) { + state.abortController = null; + } + } + /** * Session ids with a stream in flight right now. * * Read at page-hide time to attribute a departure to the turns it * actually interrupted. A live controller is the truthful test: it is - * created per request and nulled on abort, so it tracks the transport + * created per request and released on abort (`abortRequest`) or on + * stream teardown (`releaseAbortController`), so it tracks the transport * rather than the `loading` flag, which other code also drives. */ streamingSessionIds(): string[] { diff --git a/infrastructure/gsi-inventory.json b/infrastructure/gsi-inventory.json index 59b96257..10bcf0d7 100644 --- a/infrastructure/gsi-inventory.json +++ b/infrastructure/gsi-inventory.json @@ -63,7 +63,8 @@ "system-cost-rollup": [], "system-prompts": [], "user-artifacts": [ - "SessionIndex" + "SessionIndex", + "UserArtifactsIndex" ], "user-cost-summary": [ "PeriodCostIndex" diff --git a/infrastructure/lib/config.ts b/infrastructure/lib/config.ts index 9a875560..44fd6745 100644 --- a/infrastructure/lib/config.ts +++ b/infrastructure/lib/config.ts @@ -106,10 +106,9 @@ export interface ArtifactsConfig { extraFrameAncestors: string[]; // Whether recipients can *discover* artifacts shared with them (the // library's "Shared with you" tab, backed by GET /shared-artifacts). - // Default OFF and opt-in, unlike the kill-switch flags elsewhere in this - // file: the surface lands before the product decision about it. Gates the - // read only β€” the fan-out rows behind it are written unconditionally, so - // enabling this never needs a backfill. + // Default ON with a kill switch, like the other flags in this file. + // Gates the read only β€” the fan-out rows behind it are written + // unconditionally, so toggling this never needs a backfill. shareInboxEnabled: boolean; } @@ -821,14 +820,18 @@ export function loadConfig(scope: cdk.App): AppConfig { .map((s) => s.trim()).filter(Boolean) || scope.node.tryGetContext('artifacts')?.extraFrameAncestors || [], - // Default OFF, opt-in β€” the inverse of the kill-switch ternary used by - // scheduledRuns/memorySpaces/skills above. Only the literal "true" - // enables, so the empty string an unset GitHub Actions variable - // forwards resolves to off, which is the intended default rather than - // an accident. + // Default ON with a kill switch: the recipient inbox is a complete + // feature and ships enabled for every deployer (opt-out, not opt-in). + // It shipped opt-in in 1.18.0 because the surface landed ahead of the + // product decision; that decision is made, so a fork should get the + // finished feature without having to discover a variable. + // The workflow forwards `${{ vars.CDK_ARTIFACT_SHARE_INBOX_ENABLED }}`, + // which is an EMPTY STRING when the variable is unset β€” so treat + // empty/unset as "use the default (on)" and only the literal "false" + // as the off switch. (Same ternary as scheduledRuns above β€” keep in sync.) shareInboxEnabled: process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED - ? process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED === 'true' - : scope.node.tryGetContext('artifacts')?.shareInboxEnabled ?? false, + ? process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED !== 'false' + : scope.node.tryGetContext('artifacts')?.shareInboxEnabled ?? true, }, mcpSandbox: { certificateArn: process.env.CDK_MCP_SANDBOX_CERTIFICATE_ARN || scope.node.tryGetContext('mcpSandbox')?.certificateArn, diff --git a/infrastructure/lib/constructs/app-api/app-api-service-construct.ts b/infrastructure/lib/constructs/app-api/app-api-service-construct.ts index f142d11d..e681a25a 100644 --- a/infrastructure/lib/constructs/app-api/app-api-service-construct.ts +++ b/infrastructure/lib/constructs/app-api/app-api-service-construct.ts @@ -182,11 +182,11 @@ export class AppApiServiceConstruct extends Construct { environment['DYNAMODB_ARTIFACTS_TABLE_NAME'] = props.refs.artifactsTable.tableName; environment['ARTIFACTS_ORIGIN'] = props.artifactsOrigin; environment['ARTIFACTS_RENDER_TOKEN_SECRET_ARN'] = props.refs.artifactRenderTokenSecret.secretArn; - // "Shared with you" inbox. Default off; read by + // "Shared with you" inbox. Default on with a kill switch; read by // apis/shared/feature_flags.py::artifact_share_inbox_enabled, which gates // the GET /shared-artifacts route ONLY. The recipient fan-out rows are - // written by every share regardless, so flipping this on reveals a - // complete inbox rather than one that begins at the flip. + // written by every share regardless, so toggling this never reveals an + // inbox that begins at the flip. environment['ARTIFACT_SHARE_INBOX_ENABLED'] = config.artifacts.shareInboxEnabled ? 'true' : 'false'; // Skill reference-file bucket (admin-managed Skills, PR-4). Read by diff --git a/infrastructure/lib/constructs/artifacts/artifacts-data-construct.ts b/infrastructure/lib/constructs/artifacts/artifacts-data-construct.ts index 5afe2b97..480b6c19 100644 --- a/infrastructure/lib/constructs/artifacts/artifacts-data-construct.ts +++ b/infrastructure/lib/constructs/artifacts/artifacts-data-construct.ts @@ -79,6 +79,45 @@ export class ArtifactsDataConstruct extends Construct { projectionType: dynamodb.ProjectionType.ALL, }); + // Sparse index over artifact HEAD rows, keyed by owner and ordered by + // `updated_at` β€” what the library's user-wide listing is served from. + // + // The base table is already partitioned by user, so this is NOT about + // reachability: `list_for_user` could always Query PK=USER#{uid}. It is + // about what that Query has to read. HEAD and version rows share the + // partition, so the base-table Query spans roughly 3x the rows it + // returns, then filters and date-sorts them in memory β€” which also + // makes it unpaginable, since the page boundary would fall in the + // wrong place. Only HEAD rows carry GSI2PK, so this index holds one + // row per artifact, already in newest-first order. + // + // ⚠️ ADDING A SECOND GSI IS ONE `UpdateTable`, AND ONLY ONE GSI MAY BE + // ADDED PER `UpdateTable`. Do not add another index to this table in + // the same deploy β€” CloudFormation will reject the change set. See + // `test/gsi-update-limit.test.ts`. + // + // ⚠️ CFN reporting UPDATE_COMPLETE does NOT mean the index is ACTIVE: + // DynamoDB backfills it asynchronously afterwards. Anything that + // queries it must not deploy until `DescribeTable` reports + // `IndexStatus: ACTIVE`, which is why the query switch ships in a + // separate PR from this one. + // + // ⚠️ Sparse means a HEAD row without GSI2PK is ABSENT from this index + // forever, not merely stale β€” and silently. Rows written before the + // writer began stamping these keys (2026-09-04) are backfilled by + // `backend/scripts/backfill_artifact_user_index_keys.py`, which must + // have been run for the environment before anything reads the index. + this.table.addGlobalSecondaryIndex({ + indexName: 'UserArtifactsIndex', + partitionKey: { name: 'GSI2PK', type: dynamodb.AttributeType.STRING }, + sortKey: { name: 'GSI2SK', type: dynamodb.AttributeType.STRING }, + // ALL, matching SessionIndex: the library row needs title, + // content_type, version, created_at, updated_at and session_id, so a + // KEYS_ONLY projection would force a base-table read per row and + // give back exactly the amplification this index exists to remove. + projectionType: dynamodb.ProjectionType.ALL, + }); + this.bucket = new s3.Bucket(this, 'ArtifactsContentBucket', { bucketName: getResourceName(config, 'artifacts-content'), encryption: s3.BucketEncryption.S3_MANAGED, diff --git a/infrastructure/package-lock.json b/infrastructure/package-lock.json index 8d1a46d6..f883411b 100644 --- a/infrastructure/package-lock.json +++ b/infrastructure/package-lock.json @@ -1,12 +1,12 @@ { "name": "infrastructure", - "version": "1.18.0", + "version": "1.19.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "infrastructure", - "version": "1.18.0", + "version": "1.19.0", "dependencies": { "aws-cdk-lib": "2.265.0", "constructs": "10.6.0" diff --git a/infrastructure/package.json b/infrastructure/package.json index f2e3814d..5e4e28bb 100644 --- a/infrastructure/package.json +++ b/infrastructure/package.json @@ -1,6 +1,6 @@ { "name": "infrastructure", - "version": "1.18.0", + "version": "1.19.0", "bin": { "infrastructure": "bin/infrastructure.js" }, diff --git a/infrastructure/test/config.test.ts b/infrastructure/test/config.test.ts index 99c00c78..784933c7 100644 --- a/infrastructure/test/config.test.ts +++ b/infrastructure/test/config.test.ts @@ -455,6 +455,49 @@ describe('RAG Ingestion Configuration', () => { }); }); + // ============================================================ + // Artifact share inbox feature flag β€” default ON with a kill switch + // (flipped from opt-in once the surface shipped; the empty workflow + // var is the case that matters β€” see the 1.18.0 release notes) + // ============================================================ + + describe('Artifact share inbox feature flag', () => { + test('defaults to enabled when CDK_ARTIFACT_SHARE_INBOX_ENABLED is unset', () => { + delete process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED; + + expect(loadConfig(app).artifacts.shareInboxEnabled).toBe(true); + }); + + test('treats empty string (unset GitHub Actions variable) as enabled', () => { + // `${{ vars.CDK_ARTIFACT_SHARE_INBOX_ENABLED }}` renders to "" when + // unset. Under the previous opt-in default this resolved to OFF; the + // whole point of the flip is that a fork which never sets the variable + // now gets the finished feature. + process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED = ''; + + expect(loadConfig(app).artifacts.shareInboxEnabled).toBe(true); + }); + + test('CDK_ARTIFACT_SHARE_INBOX_ENABLED="false" is the kill switch', () => { + process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED = 'false'; + + expect(loadConfig(app).artifacts.shareInboxEnabled).toBe(false); + }); + + test('CDK_ARTIFACT_SHARE_INBOX_ENABLED="true" stays enabled', () => { + process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED = 'true'; + + expect(loadConfig(app).artifacts.shareInboxEnabled).toBe(true); + }); + + test('cdk.json context artifacts.shareInboxEnabled=false disables when env is unset', () => { + delete process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED; + app.node.setContext('artifacts', { shareInboxEnabled: false }); + + expect(loadConfig(app).artifacts.shareInboxEnabled).toBe(false); + }); + }); + // ============================================================ // Memory Spaces feature flag β€” default ON with a kill switch // (complete feature; ships enabled for forkers, empty var must not disable) diff --git a/tui/pyproject.toml b/tui/pyproject.toml index 7ec85aad..c5652247 100644 --- a/tui/pyproject.toml +++ b/tui/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "agentcore-tui" -version = "1.18.0" +version = "1.19.0" requires-python = ">=3.11" description = "Terminal client for the AgentCore Public Stack β€” streaming AI chat in your terminal" readme = "README.md" diff --git a/tui/src/agentcore_tui/__init__.py b/tui/src/agentcore_tui/__init__.py index fa6ceda2..bff8d598 100644 --- a/tui/src/agentcore_tui/__init__.py +++ b/tui/src/agentcore_tui/__init__.py @@ -7,6 +7,6 @@ from __future__ import annotations -__version__ = "1.18.0" +__version__ = "1.19.0" __all__ = ["__version__"] diff --git a/tui/uv.lock b/tui/uv.lock index 5fb69093..bb665a8d 100644 --- a/tui/uv.lock +++ b/tui/uv.lock @@ -8,7 +8,7 @@ resolution-markers = [ [[package]] name = "agentcore-tui" -version = "1.18.0" +version = "1.19.0" source = { editable = "." } dependencies = [ { name = "httpx" },