diff --git a/.github/docs/deploy/step-05-verify.md b/.github/docs/deploy/step-05-verify.md index 364f9b56e..5ed01dd52 100644 --- a/.github/docs/deploy/step-05-verify.md +++ b/.github/docs/deploy/step-05-verify.md @@ -102,6 +102,67 @@ In order of likelihood: --- +### 6. Subscribe to Platform Alarms (required — not automated) + +The deploy creates one SNS topic that **every** CloudWatch alarm in the stack +publishes to. It has no subscribers until you add them, so until you do this step +the alarms change colour in the console and tell nobody. + +This is deliberately not infrastructure-as-code. Several teams usually need to +hear about failures, and their membership changes far more often than the +infrastructure does — requiring a pull request, a review, and a CloudFormation +deploy to add one email address is how a notification list goes stale and stops +being trusted. Subscribing is a one-line command that touches no code. + +Find the topic and subscribe: + +```bash +PREFIX="your-project-prefix" # the same CDK_PROJECT_PREFIX you deployed with + +TOPIC=$(aws ssm get-parameter \ + --name "/${PREFIX}/observability/alarm-topic-arn" \ + --query Parameter.Value --output text) + +aws sns subscribe \ + --topic-arn "$TOPIC" \ + --protocol email \ + --notification-endpoint platform-team@example.edu +``` + +AWS sends a confirmation email; the subscription is inactive until the recipient +clicks the link. Repeat for each address or distribution list. + +Other useful protocols: + +| Protocol | Use for | +|---|---| +| `email` | A team distribution list. Simplest, and enough for most forks. | +| `https` | PagerDuty, Opsgenie, ServiceNow, or any webhook receiver. | +| `sms` | Genuine paging. Costs per message. | +| `lambda` | Custom routing, e.g. severity-based fan-out or Slack formatting. | + +Verify it took: + +```bash +aws sns list-subscriptions-by-topic --topic-arn "$TOPIC" \ + --query 'Subscriptions[].[Protocol,Endpoint,SubscriptionArn]' --output table +``` + +A `SubscriptionArn` of `PendingConfirmation` means the email has not been +confirmed yet. + +Then open the health dashboard — `{PREFIX}-platform-health` in the CloudWatch +console — and confirm the alarm-status row is populated and green. Row 1 tells +you whether traffic is being served, row 2 tells you why, and row 3 lists every +alarm's current state. + +> **Note on latency alarms:** the chat path uses server-sent events, so response +> times of tens of seconds are normal for a healthy agent turn. Latency alarms are +> deliberately set at 120 seconds. A *drop* in latency can actually mean turns are +> failing early. + +--- + ## You're Done! Your AgentCore Public Stack is deployed and running. Here's what you have: diff --git a/.github/workflows/platform.yml b/.github/workflows/platform.yml index f9bd1c17c..37d1f6298 100644 --- a/.github/workflows/platform.yml +++ b/.github/workflows/platform.yml @@ -207,6 +207,26 @@ jobs: # (default 100). Leave unset to take those defaults. CDK_MANAGED_KB_STORAGE_ALARM_GB: ${{ vars.CDK_MANAGED_KB_STORAGE_ALARM_GB }} CDK_MANAGED_KB_DAILY_COST_ALARM_USD: ${{ vars.CDK_MANAGED_KB_DAILY_COST_ALARM_USD }} + # Observability. All optional — unset means the default in config.ts. + # XRAY_SAMPLING_RATE is a rate (0.0-1.0), not a percentage. + CDK_OBSERVABILITY_ALARM_TOPIC_ENABLED: ${{ vars.CDK_OBSERVABILITY_ALARM_TOPIC_ENABLED }} + CDK_OBSERVABILITY_LOG_RETENTION_DAYS: ${{ vars.CDK_OBSERVABILITY_LOG_RETENTION_DAYS }} + CDK_OBSERVABILITY_ALB_TARGET_5XX_THRESHOLD: ${{ vars.CDK_OBSERVABILITY_ALB_TARGET_5XX_THRESHOLD }} + CDK_OBSERVABILITY_ALB_P99_LATENCY_MS: ${{ vars.CDK_OBSERVABILITY_ALB_P99_LATENCY_MS }} + CDK_OBSERVABILITY_AGENTCORE_LATENCY_MS: ${{ vars.CDK_OBSERVABILITY_AGENTCORE_LATENCY_MS }} + CDK_OBSERVABILITY_AGENTCORE_ERROR_THRESHOLD: ${{ vars.CDK_OBSERVABILITY_AGENTCORE_ERROR_THRESHOLD }} + CDK_OBSERVABILITY_LAMBDA_ERROR_THRESHOLD: ${{ vars.CDK_OBSERVABILITY_LAMBDA_ERROR_THRESHOLD }} + CDK_OBSERVABILITY_LAMBDA_DURATION_PERCENT_OF_TIMEOUT: ${{ vars.CDK_OBSERVABILITY_LAMBDA_DURATION_PERCENT_OF_TIMEOUT }} + CDK_OBSERVABILITY_DYNAMO_THROTTLE_THRESHOLD: ${{ vars.CDK_OBSERVABILITY_DYNAMO_THROTTLE_THRESHOLD }} + CDK_OBSERVABILITY_ECS_CPU_PERCENT: ${{ vars.CDK_OBSERVABILITY_ECS_CPU_PERCENT }} + CDK_OBSERVABILITY_ECS_MEMORY_PERCENT: ${{ vars.CDK_OBSERVABILITY_ECS_MEMORY_PERCENT }} + CDK_OBSERVABILITY_XRAY_SAMPLING_RATE: ${{ vars.CDK_OBSERVABILITY_XRAY_SAMPLING_RATE }} + CDK_OBSERVABILITY_XRAY_SAMPLING_RESERVOIR: ${{ vars.CDK_OBSERVABILITY_XRAY_SAMPLING_RESERVOIR }} + CDK_OBSERVABILITY_XRAY_INSIGHTS_NOTIFICATIONS: ${{ vars.CDK_OBSERVABILITY_XRAY_INSIGHTS_NOTIFICATIONS }} + CDK_OBSERVABILITY_AGENTCORE_APPLICATION_LOGS_ENABLED: ${{ vars.CDK_OBSERVABILITY_AGENTCORE_APPLICATION_LOGS_ENABLED }} + CDK_OBSERVABILITY_PROMPT_CACHE_AVOIDABLE_MISS_THRESHOLD: ${{ vars.CDK_OBSERVABILITY_PROMPT_CACHE_AVOIDABLE_MISS_THRESHOLD }} + CDK_OBSERVABILITY_PROMPT_CACHE_WASTED_USD_THRESHOLD: ${{ vars.CDK_OBSERVABILITY_PROMPT_CACHE_WASTED_USD_THRESHOLD }} + CDK_OBSERVABILITY_PROMPT_CACHE_SESSION_WASTED_USD_THRESHOLD: ${{ vars.CDK_OBSERVABILITY_PROMPT_CACHE_SESSION_WASTED_USD_THRESHOLD }} # Secrets AWS_ROLE_ARN: ${{ secrets.AWS_ROLE_ARN }} AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }} diff --git a/.kiro/specs/managed-kb-migration/HANDOFF.md b/.kiro/specs/managed-kb-migration/HANDOFF.md index ca16cfbcd..3ccf6c8c4 100644 --- a/.kiro/specs/managed-kb-migration/HANDOFF.md +++ b/.kiro/specs/managed-kb-migration/HANDOFF.md @@ -1,9 +1,39 @@ # Managed KB Migration — Handoff -**Last updated:** 2026-08-26 (groups 11–13, group 14 backend half, tag contract, **14.3 upgrade UX + enrolment surface**) · **Branch:** `feature/kb-migration` · **Nothing deployed** +**Last updated:** 2026-08-31 14:30 · **Shipped to production in 1.16.0, inert behind flags** · +**A migration has completed end to end in dev; adding a document to it needed one more IAM action** -Working state for this feature so a fresh session can pick it up without re-deriving -anything. Read this, then `tasks.md`. +Working state for this feature so a fresh session can pick it up without +re-deriving anything. Read this, then `tasks.md`. + +--- + +## 0. Read this first + +Four things invalidate earlier versions of this document: + +1. **It is deployed.** The feature shipped to production in release 1.16.0 and the + platform deploy succeeded on 2026-08-28, so `GSI7`, the Bedrock service role and + the four Lambdas exist in **both** dev and prod. Earlier revisions of this file + said "Nothing deployed"; that is no longer true. +2. **Eleven defects were found only by running it**, each reviewed clean and + deployed clean. They are §5 items 25–36 and they are the most useful part of + this document. Items 32–36 all trace to one root cause — the two engines were + never made exclusive — and are fixed in PR #900. +3. **The `document_id` "known unknown" was a false alarm** and is now resolved with + measurements — see §6. An earlier revision listed it as the top open risk. The + probe was reading facade keys that have never existed. Two genuine findings came + out of checking it (§5.32, §5.33), both still open. +4. **Iterate locally, but do not trust it for IAM.** + `scripts/local-dev/run-kb-migration.py` drives the whole state machine + in-process against dev with your SSO credentials. Three of the + five defects would have been minutes of work instead of a merge → image build → + deploy → 15-minute-tick cycle each. Use it. + + But your SSO identity is **broader than every Lambda role**, so the driver is + structurally blind to IAM gaps — that is how §5.31 shipped after a migration had + already "completed end to end". Get the logic right locally; prove the + permissions by deploying and letting the real roles do the work. --- @@ -11,13 +41,48 @@ anything. Read this, then `tasks.md`. | | | |---|---| -| Spec | Complete, audited 3× to clean. 25 requirements, 201 criteria, 0 dangling refs | -| Implementation | Groups **1–13** done, plus group 14 except 14.5. 14.4's one-click document retry is deferred. 4 subtasks left: 14.4's retry, 14.5, and group 15 | -| Tests | 617 infra (jest) · **6,603** backend (pytest, 6 m 20 s) · **1,886** frontend (vitest, 7 s) · 5 pre-existing unrelated failures | -| Deployed | **Nothing.** No `cdk deploy`, no AWS mutation, at any point | -| Feature flags | `migrationEnabled` **on in development**, off in production. `newDefault` and `reconcilerArmed` off in both (explicit `false`, set as GitHub Environment variables) | +| Spec | Complete. Requirement **8.5 was amended by measurement on 2026-08-31** — see §5.29 | +| Implementation | Groups 1–14 except 14.5. A migration has completed `shadow → verify → promote → retain` in dev and serves from the managed backend | +| Tests | 640 infra (jest) · ~6,780 backend (pytest) · 1,936 frontend (vitest) · 5 pre-existing unrelated Strands failures | +| Deployed | **dev and prod.** Flags off in prod; `migrationEnabled` on in dev | +| Open PRs | **#900** — engine exclusivity: the legacy pipeline stands down for a promoted KB (§5.32, §5.34) and deletion propagates to the managed engine (§5.36). Four CI checks green. Merging triggers **both** `backend.yml` (rag-ingestion + kb-sync images) and `platform.yml` (two new IAM grants). · **#899** — this document. · #898 merged as `ef2f4c9e` | +| Uncommitted | none — working tree clean as of 2026-08-31 14:30 | + +### Flag state (GitHub Environment variables) + +| Flag | development | production | +|---|---|---| +| `CDK_MANAGED_KB_MIGRATION_ENABLED` | `true` | `false` | +| `CDK_MANAGED_KB_NEW_DEFAULT` | `false` | `false` | +| `CDK_MANAGED_KB_RECONCILER_ARMED` | `false` | `false` | +| `CDK_TAG_ENVIRONMENT` | `dev` | `prod` | + +`newDefault` has **no reader anywhere in `backend/src`** — "new knowledge bases are +created managed" is design §14.7 steps 5–8, a follow-up spec. Setting it does +nothing, which is worth knowing before someone flips it expecting an effect. + +⚠️ **Production carries every defect fixed after 1.16.0 shipped.** It cannot fire, +because nothing enrols while `migrationEnabled` is false. Do not turn that flag on +in prod until #898 and the uncommitted work have landed and shipped. + +### Commits + +**On `fix/kb-legacy-pipeline-engine-gate` — PR #900, open:** + +``` +e3398f30 propagate document deletion to the managed knowledge base (§5.36) +8a35dbc6 the legacy pipeline stands down for a promoted knowledge base (§5.32, §5.34) +``` + +**Merged as `ef2f4c9e` (was PR #898):** + +``` +fdf15d21 grant bedrock:StartIngestionJob, which authorizes direct ingestion (§5.31) +6420f148 drop the embedding pin, defer verify, complete a migration in dev (§5.29, §5.30) +7542d907 record the knowledge base id before anything else can fail (§5.28) +``` -### Commits (16 on the branch, all pushed) +**Already merged (16, on develop):** ``` 45239838 one source of truth for the managed KB tag contract @@ -39,9 +104,9 @@ ffa7a408 KB_Record data layer with conditional state transitions (group 5f2c98b1 spec, schema and worker platform (groups 1, 2) ``` -**Uncommitted working tree:** the 14.3 upgrade surface — `apis/app_api/kb_upgrade/`, -two transitions appended to `kb_backend/records.py`, the Angular card and service, -three test files. See §7 for the file map and §2 for how to run it. +**Working tree: clean.** An earlier revision listed the 14.3 upgrade surface and +then the 8.5 amendment as uncommitted; both have landed. `apis/app_api/kb_upgrade/` +is merged — see §7 for the file map and §2 for how to run it. ### Is the feature reachable yet? @@ -51,14 +116,31 @@ knowledge base, which writes a `KB#` record in `shadow` with the GSI7 work keys. Before it, nothing wrote either, so every group could have been finished with the feature unreachable (§5 defect 21). -What is still missing is the **worker's deployment**, not its code. The dispatcher -and worker are Lambdas behind an undeployed image, so an enrolled record sits in -`shadow` indefinitely and the card shows perpetual progress. That is the correct -local behaviour, not a bug. +The worker's image **is deployed** — PR #886 shipped `Dockerfile.kb-migration` and +all four Lambdas run real handlers. An earlier revision of this section said the +image was undeployed and a `shadow` record would sit forever; that is no longer +true. The dispatcher's rule is `ENABLED` and ticking every 15 minutes in dev. + +⚠️ **That tick is a hazard while #898 is unmerged.** The deployed worker predates +it, so an enrolled record can be picked up by pre-fix code and failed at `verify`. +Always drive a local migration with `--break-lease`, which defers `dueAt` 20 minutes +out so the deployed dispatcher skips it. -**Three behaviour changes ARE live on the existing path** and are the only things -worth testing by hand right now: -1. The document-status filter now **fails closed** (group 6). +### What works today, verified live in dev + +| | | +|---|---| +| Chat against a promoted KB | **Yes.** `ast-1a90784a7f18` is `retain` / `managed` and serves real chunks | +| Add a document to a promoted KB | **Yes, once #898 deploys.** Blocked before that by §5.31. The consumer's EventBridge rule is `ENABLED`, the bucket has EventBridge notification on, and `grantRetrieval` is attached for the retrievability poll | +| Create a knowledge base that is managed from birth | **No — not implemented.** `newDefault` has **zero readers** in `backend/src` (grep for `NEW_DEFAULT`, `newDefault`, `new_default`: no matches). Design §14.7 steps 5–8, a follow-up spec. The only route onto the managed engine is enrol → migrate | +| Image-only PDFs | **Managed yes, legacy no.** A pure-diagram flowchart fails Docling outright (`zero chunks` → `failed`, permanently) and is served fine by managed via `imageExtractionStatus: ENABLED` (§5.35) | +| Deleting a document | **Propagates to managed only after #900 deploys.** Before that the managed copy is orphaned; the fail-closed status filter is what keeps it unserved (§5.36) | +| Whole local chain | **Yes.** SPA :4200 → app_api :8000 → inference_api :8001. `chat-http.service.ts` posts to `{appApiUrl}/chat/stream`; the app_api proxy forwards to `INFERENCE_API_URL`, which defaults to and is set to `http://localhost:8001`. Retrieval runs at `inference_api/chat/routes.py:1814`, so the RAG code answering a local chat is the code on disk | + +**Three behaviour changes are live on the existing legacy path** regardless of any +flag: +1. The document-status filter now **fails closed** (group 6) — except for the one + fail-open line in §5.33. 2. Retrieval queries are **clamped to 10,000 chars** (group 5). 3. Retrieval requires a resolved access grant (group 11). Both production callers pass one; the parameter is required and keyword-only, so a third caller added @@ -75,7 +157,7 @@ macOS host, tooling installed locally. **There is no devcontainer.** ```bash # infrastructure cd infrastructure && npm run build # tsc -cd infrastructure && npx jest # 611 passing +cd infrastructure && npx jest # 640 passing, 30 suites # backend cd backend && uv run python -m pytest tests/ -q # 6 m 20 s, 6,603 passing @@ -91,6 +173,77 @@ There is **no eslint config** in the repo despite the steering docs mentioning ESLint; `npx eslint` fails with "couldn't find an eslint.config.*". Type-check with `tsc --noEmit` and build with `ng build` instead. +### Driving a migration locally (do this before deploying anything) + +```bash +cd backend +uv run python ../scripts/local-dev/run-kb-migration.py --show +uv run python ../scripts/local-dev/run-kb-migration.py --break-lease +``` + +Runs the worker's steps in-process against dev with your SSO credentials, so the +whole state machine iterates in seconds. `--break-lease` clears the 15-minute lease +between steps and defers `dueAt` 20 minutes out so the deployed dispatcher does not +race you. + +It needs five variables in `backend/src/.env` that the app-api task definition does +not carry — copy them from the deployed worker Lambda: +`MANAGED_KB_SERVICE_ROLE_ARN`, `MANAGED_KB_TAG_VALUE_PREFIX`, +`MANAGED_KB_TAG_VALUE_ENVIRONMENT`, `MANAGED_KB_METRIC_NAMESPACE`, +`KB_MIGRATION_RETAIN_DAYS`. + +**What it does not prove:** the worker Lambda's IAM role (your SSO identity is +broader), the CDK environment wiring, or the image contents. Those are deploy-time +concerns — check them by deploying. Getting the logic right here first is the point, +and two of the five defects below were IAM/wiring and could only surface that way. + +### Two read-only diagnostics worth knowing before you debug anything + +```bash +cd backend +# Per-document ingestion timing, and WHICH engine did the work. +uv run python ../scripts/local-dev/kb-doc-timings.py + +# The same query through BOTH engines, side by side. +uv run python ../scripts/local-dev/kb-compare-engines.py "CS434" +uv run python ../scripts/local-dev/kb-compare-engines.py -f queries.txt +``` + +`kb-doc-timings.py` derives the engine from the record rather than guessing: +`chunkCount`/`vectorStoreId` are only ever written by the legacy pipeline, +`indexedAt`/`retrievableAt` only by the managed consumer, so a document carrying +both was double-indexed (§5.32). Measured on a 132 KB PDF: legacy `complete` at +30 s, managed retrievable at **95 s**, `INDEXED → retrievable` **0.9 s** — which +independently confirms the evaluation's 0.75–1.03 s. + +⚠️ It reports legacy as `(lost)` for a double-indexed document, and that is a real +limit, not a bug: the managed consumer overwrites `updatedAt`, so the legacy +finish time is unrecoverable afterwards. Capture it live if you need it. + +`kb-compare-engines.py` exploits the `retain` window — promotion moves no data, so +for 30 days **both** indexes hold the corpus and the same query can be put to +both. Marks each chunk `[in ]`/`[CUT]` against `MAX_CONTEXT_CHARS`, which is the +detail that makes the comparison meaningful: only ~2,000 characters reach the +model, so in practice **one or two chunks**, and precision@1 is nearly the whole +game. Absolute scores are not comparable across engines (legacy is a negated +cosine distance); order and **spread** are. + +The clearest measured difference, and the one to reach for first — exact-token +search, where legacy is pure vector and managed is hybrid: + +| query `CS434` | top chunk | spread | +|---|---|---| +| legacy | `SECTION 4: TECHNICAL ELECTIVES…` (does not contain CS434) | **0.0561** | +| managed | `- Algorithms of Machine Learning (3) CS434 - Applied Deep…` | **0.4988** | + +Legacy's five chunks sit within 0.056 of each other — flat, so its ranking is +close to arbitrary. Nine times the separation on managed, with the literal match +first. + +⚠️ **PR #900 ends this trick.** Once the legacy pipeline stands down for a +promoted knowledge base, new uploads land in the managed index only, so a +like-for-like A/B needs two assistants or documents that predate promotion. + ### Running the upgrade UI locally ```bash @@ -159,6 +312,25 @@ stack at module scope. Pulling that into a Lambda image blows the size budget. - Enforced by `backend/tests/architecture/test_kb_backend_boundary.py`. - `apis.shared.embeddings` is a *separate* package and is fine to use. +### AWS authorizes some Bedrock APIs under a *different* action name ⚠️ + +Three times now, a grant listing exactly the API the code calls has deployed clean, +reviewed clean, and failed on first real use: + +| API called | IAM action actually checked | Symptom | +|---|---|---| +| `CreateKnowledgeBase` with tags | `bedrock:TagResource` | fails the moment a real KB is created (§5.26) | +| (reconciler tag read) | `bedrock:ListTagsForResource` | fails closed — every KB looks untagged, orphan sweep reports a clean account forever (§5.26) | +| `IngestKnowledgeBaseDocuments` | **`bedrock:StartIngestionJob`** | every document upload `AccessDenied` (§5.31) | + +The action name and the API name are not the same namespace. **Check the service +authorization reference or the feature's own prerequisites page before assuming a +grant is complete**, and never infer completeness from a successful local run — SSO +identities are broader than every Lambda role here. + +Corollary: an action in a grant that no code calls is not necessarily dead. Read the +docblock before deleting it. + ### Module constants must be read at call time Never `def f(timeout=MODULE_CONSTANT)`. Python binds default arguments once at import, @@ -367,37 +539,312 @@ saying why that number is a property of AWS rather than a knob. --- -## 6. Remaining work +### The six that only running it revealed + +These came out in sequence over 2026-08-27 to 08-31, each one step further into the +saga than the last. Every one reviewed clean, deployed clean, and did nothing or +failed on first real use. If you read only one section of this file, read this one. + +25. **The dispatcher could not find the worker.** The construct set + `MANAGED_KB_WORKER_FUNCTION_NAME`; `dispatcher.py` reads + `KB_MIGRATION_WORKER_FUNCTION_NAME` — the convention its siblings use. Every + tick raised `RuntimeError: ... is not set`, on a fifteen-minute schedule, in + silence unless someone read the logs. `kb-sync` does not have this bug for + exactly one reason: `kb-sync.test.ts` asserts the variable exists. + A second mismatch found by the same sweep: the construct published + `MANAGED_KB_RETENTION_WINDOW_DAYS`, which nothing reads, while + `worker._retain_days()` reads `KB_MIGRATION_RETAIN_DAYS` — so Req 15.11's + configured window was silently replaced by the code's 30-day floor. **Guard:** + `tests/supply_chain/test_kb_migration_env_contract.py` now asserts every + `os.environ` name the handlers read is set by the construct, and that the + construct publishes nothing unread. + +26. **`bedrock:TagResource` was not granted.** `CreateKnowledgeBase` is called + *with* tags and AWS authorises the tagging as a separate action, so the grant + reviewed as complete and failed the moment a real knowledge base was created. + `bedrock:ListTagsForResource` was missing for the same reason, with a quieter + failure: the reconciler fails closed on a tag read, so every knowledge base + would look untagged and the orphan sweep would report a clean account forever. + +27. **`CreateDataSource` ran against a `CREATING` knowledge base.** + `CreateKnowledgeBase` returns before the knowledge base is usable — this + module's own header records 47–124 s to `ACTIVE` — and the code called + `CreateDataSource` immediately. `ConflictException` is deliberately not + retryable and `_call`'s backoff tops out near 60 s, so the wait had to be + explicit. **Why no test caught it:** `FakeBedrockAgent.create_knowledge_base` + returned `status: "ACTIVE"`, which the real API never does. + +28. **The knowledge base id was never recorded until both creates succeeded.** + `attach_aws_ids` needs both identifiers, so a failure between them left a + record with no `awsKbId` — and every later attempt re-entered the create path + and was refused, permanently, because the *name* was taken. **The + `clientToken` does not save this**: AWS idempotency tokens expire within + minutes. Earlier revisions of this document and of the module header claimed + otherwise; they were wrong. Fixed by `records.attach_knowledge_base_id` + (persist immediately) plus adopt-by-name for records already stuck. + **Why no test caught it:** two tests *certified* the bug — + `test_the_record_survives_as_a_discoverable_retry_anchor` asserted + `"awsKbId" not in anchor`, and its sibling relied on the fake modelling + `clientToken` dedup as **permanent** while not modelling name uniqueness at + all. The fake now enforces name uniqueness and treats tokens as expired by + default. + +29. **The embedding pin and managed reranking are mutually exclusive.** Req 8.5 + pinned `titan-embed-text-v2:0` via `embeddingModelType: CUSTOM`; Req 11.2 + requires `rerankingModelType: MANAGED`. AWS rejects the combination, and the + §13 evaluation had measured the two **separately, never together**. Req 8.5 is + now amended: the pin protected a failure mode that cannot occur in managed mode + (we never embed the query — Bedrock embeds both sides), and the evaluation + measured the pin as worth nothing while reranking measurably separates scores. + Confirmed in dev: pinned + `NONE` gives flat 1.00/0.982/0.952; unpinned + + `MANAGED` gives 0.413/0.199. + +30. **`verify` failed a good migration for being asked too early.** The canary + retrieval returned nothing because the freshly-ingested document was not yet + queryable, and that was treated as terminal. Measured: **~45 s** from ingest to + retrievable on a fresh knowledge base, against the docstring's "0.75–1.03 s" + (a warm-knowledge-base figure). `verify` now defers via + `records.defer_verify`, bounded at `MAX_VERIFY_ATTEMPTS`. Adoption also learned + to skip knowledge bases in `DELETING`, found the same way: a local recreate + adopted one mid-delete. -| Group | Subtasks | Notes | -|---|---|---| -| **14** Surfaces | 1½ | **14.5** admin surface (filter by engine, stored bytes + document counts, bulk migrate, per-KB retry) — not started. **14.4** is surfaced but its one-click document retry is deferred; see the deferral below. 14.0–14.3, 14.6, 14.7 are **done**. | -| **15** Pre-promotion verification | 3 | The gate before any real traffic moves. | +--- -### Known deferrals (correct, not oversights) +### The seventh, from adding a document rather than migrating one + +31. **`bedrock:StartIngestionJob` was not granted, so direct ingestion could not + run at all.** Found by uploading a second document to the already-promoted + knowledge base in dev. The `DOC#` record went to `failed` carrying: + + ``` + AccessDeniedException ... IngestKnowledgeBaseDocuments ... not authorized + to perform: bedrock:StartIngestionJob on resource: knowledge-base/M8WQZVQJ8X + ``` + + AWS authorises `IngestKnowledgeBaseDocuments` under the **adjacent action + name** `bedrock:StartIngestionJob`. Both are listed in one statement in AWS's + direct-ingestion prerequisites + (`bedrock/latest/userguide/kb-direct-ingestion-prereq.html`). + `grantManagedKbDirectIngestion` carried only the name matching the API call, + so it reviewed as complete, deployed clean, and failed on first real use — + identical in shape to §5.26's missing `bedrock:TagResource`. Third occurrence + of that pattern; assume a fourth exists. + + **The worker had the same gap.** It receives the same grant and calls the same + API, so the first migration driven by the *deployed* dispatcher would have + failed the same way. It stayed hidden because every migration to date was + driven by `run-kb-migration.py` under an SSO identity broader than either + Lambda role — the exact limitation §2 names. The local driver cannot find this + class of defect, ever. Only a deployed run can. + + ⚠️ **The action looks like a mistake and is not.** Requirement 9.2 forbids + *calling* `StartIngestionJob` (0.1 RPS account-wide — one document per ten + seconds) and nothing calls it. Holding it is authorisation, not invocation. A + docblock on the grant and a separately-named test carry that reason so the + obvious cleanup fails a test that explains itself. + `bedrock:ListKnowledgeBaseDocuments` is in AWS's example policy and omitted + deliberately: no code path calls it. + + Guards: three tests across `managed-kb.test.ts` and `kb-migration.test.ts`, + one asserting **both** the worker and ingestion-consumer roles carry it. + Mutation-tested — removing the action fails exactly four tests, each named for + the reason. -- **One-click document reprocess (Req 21.2).** Ingestion is S3-event-triggered - (`documents/ingestion/handler.py`) and there is **no reprocess endpoint** — the - only document writes are upload-url, import, upload-failed and delete. A retry - control therefore needs new backend that re-fires the pipeline against bytes - already in S3, which is a change to a live ingestion path. Deliberately not - improvised. The card directs the user to re-upload via "Add files", a retry path - that works today. **Close by building the endpoint or by amending Req 21.2 to - accept re-upload** — do not leave it ambiguous. -- **`backend/Dockerfile.kb-migration`** does not exist yet, on purpose. The real image - needs five artefacts that do not exist: the handler modules, their - `requirements.txt`, a case in `scripts/build/build-one.sh`, `backend.yml` jobs, and - entries in the **hand-maintained** lists in - `backend/tests/supply_chain/test_dockerfile_pinning.py` and - `test_lambda_image_imports.py`. Per platform-as-bootstrap, CDK ships the bootstrap - stub and the **workflow** ships the real image. -- **Reconciler EventBridge wiring** (Reqs 14.1, 14.7) — `infrastructure/`, platform - group. Backend code never deploys before the IAM and resources it requires. -- **Group 7's snapshot reservation now has its caller** (`run_shadow`), reserving - the whole corpus before anything is provisioned. +--- + +### The eighth through eleventh, all from one evening of running it — fixed in PR #900 + +These four are one root cause wearing four costumes: **the two engines were never +made exclusive.** The consumer stands down for a legacy document; nothing made the +legacy pipeline stand down for a managed one, and nothing propagated a deletion to +the managed engine at all. Every symptom below follows from that. + +32. ✅ **Routing exclusivity was enforced on only one side, so a document added to + a managed knowledge base was indexed twice.** design.md §537 states "a + document is indexed on exactly one backend outside a deliberate migration or + dual-read pilot, so no double-indexing", and task 9.2 claims tests for it. The + consumer does return immediately for a legacy document. But the **legacy + handler had no engine gate at all**, and its `s3:ObjectCreated:*` notification + on `assistants/` is still live alongside the (now enabled) EventBridge rule. + Both fired. + + Visible in real data on `DOC#DOC-dc8b65658e29`: `chunkCount: 8` and + `vectorStoreId: assistants-index` written by the legacy pipeline, while the + managed side failed with §5.31. + + **Why no test caught it:** every exclusivity test in + `test_kb_ingestion_consumer.py` is on the consumer's side + (`test_a_legacy_document_is_not_ingested_here` and siblings). Nothing asserted + the legacy handler skips a managed document, which is precisely the half that + did not exist. A test suite can be thorough about the side that works. + + Fixed: `handler.py` resolves the engine before writing anything and returns + early for `managed`. An earlier revision of this entry said the gate was + missing from "both copies" including + `infrastructure/bootstrap-assets/rag-ingestion/handler.py` — that was + misleading. The bootstrap copy is a 33-line no-op placeholder that indexes + nothing and needs no gate. + +33. ⚠️ **STILL OPEN. The document-status filter has one fail-*open* line in an + otherwise fail-closed function.** `_filter_vectors_by_document_status` opens + with `if not doc_ids: return vectors` — so a batch of chunks carrying no + `document_id` at all bypasses the DynamoDB check entirely and is served + unverified. Every other unprovable path in that function returns `[]` and + emits `METRIC_STATUS_FILTER_FAIL_CLOSED`. + + Not firing today: managed chunks do carry the id (§6 below), and legacy chunks + always have. It predates this feature. But `_document_id` returns `""` when + `location.customDocumentLocation.id` and both metadata mirrors are absent, + which is exactly the input that would trip it, and the failure is silent in + the serving direction. + +34. ✅ **Two writers owned one `status` field, so "ready" was decided by a + coin toss.** The sharpest consequence of §5.32, and worth its own entry + because the duplicate vectors are the cheap half of that defect. + + Both outcomes were observed within an hour of each other in dev: + + | document | what happened | + |---|---| + | `DOC-b5d5d8019f44` | legacy wrote `complete` at **+30 s**; the managed KB could not answer until **+95 s**. Sixty-five seconds of "your document is ready" followed by an answer that does not mention it. | + | `DOC-d637491d6cb1` | Docling produced **zero chunks** → `failed`. Bedrock indexed it fine and served it. It only ended up reading `complete` because the consumer finished **second**. | + + That second row is the alarming one, and the ordering was luck: reverse it — + purely a function of parse time — and a good, retrievable document reads + `failed` permanently, with no retry endpoint to recover it (task 14.4 open). + + It also defeated the exact protection `ingestion_consumer.py` documents in its + header: *"the UI says the upload worked, the user asks a question straight + away, and the answer does not mention their document."* The consumer polls + until the document is genuinely retrievable to prevent that. A second, + ungated writer undid it in one line. + + ⚠️ **The lesson generalises past this feature: any field two components can + write needs a stated owner.** Nobody chose this race; it appeared because a + new writer was added beside an old one and the question was never asked. + +35. ✅ **Bedrock's image extraction works, and it makes the legacy pipeline's + hard failure visible.** Not a defect in the new code — a capability + difference nobody had measured, found while testing an image-only PDF. + + A 4-year curriculum flowchart (465 KB, pure diagram, no text layer) on a + **legacy** assistant: `ValueError: Docling produced zero chunks`, status + `failed`, permanently unusable — `docling_processor.py` sets `do_ocr=False` + and `generate_page_images=False`. On the **promoted** assistant the same file + was retrievable in 94.5 s, and the chunks show Bedrock's vision model output + (` Hierarchical and Timeline Diagram`), including + prerequisite chains that exist nowhere in the document as text. + + `imageExtractionStatus: ENABLED` is set by `provisioning.py` and was confirmed + live on the dev data source. Note the old bundled `aws` CLI does **not** echo + `mediaExtractionConfiguration` back from `get-data-source` — it looked + unset until re-read with the pinned boto3. Do not conclude a field was not + applied from CLI output alone. + +36. ✅ **Deletion was never propagated to the managed engine, so a promoted + corpus could only grow.** `cleanup_service` removed the legacy S3 Vectors copy + and the `DOC#` row and never touched the managed knowledge base. The managed + delete path existed — `kb_backend/tombstones.py` — but its only callers are in + the reconciler, which is report-only with `reconcilerArmed` off. + + Three consequences, none of which raised anything: + + * storage paid for indefinitely at **$5.00/GB-month** against S3 Vectors' + ~$0.15 — orphans on the expensive engine; + * **silent retrieval degradation**, because the status filter runs *after* + retrieval: each orphan consumed a slot in `top_k` and was then dropped, so a + query could return five chunks and the model see two; + * the only thing preventing deleted content from being **served** was the + fail-closed status filter — which made §5.33's one fail-open line + load-bearing in a way it was never designed to be. + + Fixed with a third, engine-gated phase in `cleanup_document_resources`, + conjoined into `all_succeeded` so a failure blocks the hard delete. + + ⚠️ **The gate here is deliberately the opposite of the ingestion gate, and + that asymmetry is the interesting part.** On ingest, an unreadable KB record + resolves to *legacy*: being wrong costs a duplicate index while the consumer + still drives the document to a correct terminal state. On delete it must + **fail**: the `DOC#` row is what the status filter joins against, so reporting + success on a failed managed delete would remove the row *and* leave the + content — the one combination that turns a storage leak into a disclosure. + Same question, opposite answers, for a reason worth keeping. + + IAM again had no code to give it away: neither the app-api task role nor the + kb-sync worker could delete from a managed knowledge base, so this would have + deployed clean and failed on first use. New `grantManagedKbDocumentDeletion`, + narrower than `grantDirectIngestion` on purpose — these callers only remove, + so a bug in the delete path cannot add content and a bug in the ingest path + cannot remove it. Fourth instance of the §3 adjacent-action pattern, so it + carries `StartIngestionJob` pre-emptively rather than waiting to be taught. + + kb-sync needed the image change too: its worker calls + `cleanup_document_resources` after soft-deleting a document whose upstream + source has vanished. --- +## 6. Remaining work + +### Do these first + +| | | +|---|---| +| **Merge #900** | Engine exclusivity, both halves. Triggers `backend.yml` (rebuilds rag-ingestion **and** kb-sync — the content hash moves because `kb_backend` was added to both images' `SOURCE_DIRS`) and `platform.yml` (the two new IAM grants). Wait for the platform deploy before testing a deletion on a promoted assistant, or the delete fails on IAM and the `DOC#` row is deliberately kept | +| **Then re-add a document in dev** | `DOC#DOC-dc8b65658e29` is parked at `failed` from §5.31 and is not retried retroactively — there is no reprocess endpoint (task 14.4). Re-upload; that path works | +| **Then drive a migration from the *deployed* dispatcher, not the local driver** | §5.31 is the proof that the local driver cannot see IAM defects: its SSO identity is broader than either Lambda role. Every remaining unknown in this feature is of that class | + +### Open, in rough order + +| Group | Notes | +|---|---| +| **§5.33** the one fail-open line | The only finding from 2026-08-31 still open. `if not doc_ids: return vectors` in `_filter_vectors_by_document_status`. Make it fail closed with `METRIC_STATUS_FILTER_FAIL_CLOSED` like every other unprovable path in that function, and pin it with a test that mutation-fails. Lower stakes now that §5.36 removes deleted content from the managed engine, but still the one silent-serving path left | +| **engine visibility** | Nothing logs *which* engine served a query — the resolver only logs on failure — so "is the new one actually working?" can only be answered from the KB record. One INFO line in the facade, plus a `Managed`/`Classic` badge in the knowledge base section, both unbuilt. Wanted before a wide rollout, because this feature's whole risk profile is silent regressions | +| **14.4** one-click document retry | Req 21.2. Ingestion is S3-event-triggered and there is no reprocess endpoint, so this needs new backend against a live pipeline. The card currently directs the user to re-upload, which works today. Close it by building the endpoint **or** by amending Req 21.2 to accept re-upload | +| **14.5** admin surface | not started. Filter by engine, stored bytes, document counts, bulk migrate, per-KB retry | +| **15.1** packaged-SDK probe | the *static* half is done and passing (`boto3==1.43.68` carries `MANAGED`, the embedding members, `FLOAT32`, all four document ops, no `AWS_DATA_PATH`). The live half has now effectively been done by hand — a real create → ingest → retrieve → promote succeeded in dev | +| **15.2** ingestion-concurrency probe | unanswered. Do not size a wide fleet migration before it | +| **15.3** full matrix | run it once the above land | + +### RESOLVED — the `document_id` / `relevance` "known unknown" was a false alarm + +An earlier revision of this file reported that +`search_assistant_knowledgebase_with_formatting` returned `document_id: None` and +`relevance: None` after promotion, and inferred that the status filter was either +not running on the managed path or losing the id. **Both inferences were wrong.** +Measured against the live dev knowledge base (`M8WQZVQJ8X`) on 2026-08-31 with the +pinned boto3 1.43.68: + +| Layer | Result | +|---|---| +| Raw `Retrieve` | `location.customDocumentLocation.id = "DOC-ae5cc5434f2d"` on both chunks | +| `ManagedKbBackend._to_chunk` | `document_id='DOC-ae5cc5434f2d'`, `relevance=0.4226 / 0.1616` | +| Facade output | `metadata.document_id = 'DOC-ae5cc5434f2d'`, `distance = −0.4226 / −0.1616` | + +The probe read `result["document_id"]` and `result["relevance"]` at the **top level** +of the facade output. Those keys have never existed. The facade has emitted exactly +four keys — `text`, `distance`, `metadata`, `key` — since the function was first +written (`git log -L` on the `formatted_results.append` block confirms it, back +through `e34d928c`). The id lives at `metadata.document_id`; relevance is exposed as +`distance`, its exact negation. Reading a key outside the contract returns `None` on +**both** backends, so the observation said nothing about the managed path. + +The status filter is genuinely running, not bypassed: it collected +`{DOC-ae5cc5434f2d}`, looked it up, found `status = "complete"`, and kept both +chunks. Verified independently — that `DOC#` record does read `complete`. + +**The lesson is about the probe, not the code.** Asserting on a response shape +nobody had checked against the producing function turned four correct layers into a +reported defect, and it was written up as the highest-priority open item. Confirm +the contract before believing a `None`. (While confirming it, §5.33 turned up as a +genuine finding in the same function.) + +### Known deferrals (correct, not oversights) + +- **Reconciler EventBridge wiring** (Reqs 14.1, 14.7) — the rule exists and is + enabled; `reconcilerArmed` stays off so it reports rather than deletes. +- **Group 7's snapshot reservation** has its caller (`run_shadow`). + ## 7. File map ``` @@ -445,6 +892,11 @@ frontend/ai.client/src/app/knowledge-base/ knowledge-base-section.component.* the card: offer / progress / notice / failure + the stranded-document disclosure +scripts/local-dev/ + run-kb-migration.py drive the real worker in-process; --break-lease + kb-doc-timings.py per-document ingest timing + which engine did it + kb-compare-engines.py one query, both engines, side by side + scripts/teardown/ managed-kb.sh delete tag-matched KBs BEFORE any stack diff --git a/.kiro/specs/managed-kb-migration/requirements.md b/.kiro/specs/managed-kb-migration/requirements.md index a543fd7ab..7d4b9b4ee 100644 --- a/.kiro/specs/managed-kb-migration/requirements.md +++ b/.kiro/specs/managed-kb-migration/requirements.md @@ -317,8 +317,42 @@ capability we are paying for. real connector type in `managedKnowledgeBaseConnectorConfiguration.connectorParameters`. 4. THE system SHALL use connector type `CUSTOM`. -5. THE system SHALL set `embeddingModelType: CUSTOM` pinned to - `amazon.titan-embed-text-v2:0` at `FLOAT32` (the service-model enum value; lowercase is rejected) and 1024 dimensions. +5. THE system SHALL NOT send an embedding pin — no `embeddingModelType`, no + `embeddingModelArn`, no `embeddingModelConfiguration` — and SHALL let Bedrock + choose and manage the embedding model for a managed knowledge base. + + > **Amended 2026-08-31, by measurement.** This criterion previously required + > `embeddingModelType: CUSTOM` pinned to `amazon.titan-embed-text-v2:0` at + > `FLOAT32` and 1024 dimensions. That was carried over from the Legacy_Backend + > without re-deriving it, and it is wrong here for two independent reasons. + > + > **It protected a failure mode that cannot occur.** On S3 Vectors *we* embed + > the user's question (`s3vectors_backend` → `apis.shared.embeddings`), so the + > query model must match the model that indexed the documents or the similarity + > search compares vectors from different spaces. Managed retrieval sends + > `retrievalQuery={"text": …}` and managed ingestion sends `inlineContent` + > text — we never produce a vector. Bedrock embeds both sides itself, so + > consistency is the service's invariant and not ours to get wrong by omission. + > + > **AWS refuses the pin together with managed reranking.** Measured against + > dev, all four combinations: + > + > | Embedding | `rerankingModelType` | Result | + > |---|---|---| + > | `CUSTOM` (pinned) | `MANAGED` | `ValidationException` | + > | `CUSTOM` (pinned) | `NONE` | ok — scores 1.00 / 0.982 / 0.952 (flat) | + > | default (managed) | `MANAGED` | ok — scores 0.413 / 0.199 (separated) | + > | default (managed) | `NONE` | ok | + > + > The pin and Requirement 11.2's reranking are therefore mutually exclusive. + > §13 measured the pin as worth nothing ("identical cold-ingest time and + > identical answer quality to the built-in embedding — 9/9 either way") and + > reranking as worth a great deal ("the reranker is what makes a small context + > cap defensible"). Keeping reranking is the side with evidence behind it. + > + > Requirement 8.8's immutability still holds and now matters more: the choice + > cannot be revisited per knowledge base after creation. + 6. THE system SHALL enable `mediaExtractionConfiguration.imageExtractionConfiguration.imageExtractionStatus = ENABLED` on the data source. diff --git a/.kiro/steering/observability.md b/.kiro/steering/observability.md new file mode 100644 index 000000000..6205c7819 --- /dev/null +++ b/.kiro/steering/observability.md @@ -0,0 +1,317 @@ +--- +inclusion: fileMatch +fileMatchPattern: ["infrastructure/lib/constructs/observability/*", "infrastructure/test/observability-*"] +--- + +# Observability + +Every CloudWatch alarm in `PlatformStack` publishes to one SNS topic. This doc +covers the rules that keep that true, the gotchas that silently break alarms, and +what to do when one fires. + +## 0. The failure this system was built to prevent + +Before this work the stack had 13 alarms and **not one of them notified anybody**. +Three separate constructs carried a comment saying so. Two of those alarms were +worse than silent: they watched metric names that exist in no CloudWatch +namespace at all, so they sat in `INSUFFICIENT_DATA` from the day they were +created — which an operator reads as *healthy*. + +Both failures share a shape: **nothing errored.** The alarm deployed, evaluated, +and turned green. Every rule below exists because this domain fails quietly, and +quiet failure has to be caught by structure or by a test, never by remembering. + +## 1. Never call `new cloudwatch.Alarm()` — use `AlarmFactory` + +```typescript +const alarms = new AlarmFactory(this, config, props.alarmTopic); + +alarms.alarm('MyAlarmLogicalId', { + name: 'my-service-errors', // NOT alarmName; prefix is applied for you + alarmDescription: 'What broke, and what the first response should be', + metric: someMetric, + threshold: config.observability.someThreshold, + evaluationPeriods: 2, + comparisonOperator: cloudwatch.ComparisonOperator.GREATER_THAN_THRESHOLD, + treatMissingData: cloudwatch.TreatMissingData.NOT_BREACHING, +}); +``` + +The factory attaches both `AlarmActions` and `OKActions` as a consequence of +being used at all. `new cloudwatch.Alarm()` produces a console-only alarm that +looks completely finished, which is why the rule is enforced by a source-level +test rather than convention: + +- `observability-alarm-routing.test.ts` fails if any file under `lib/` calls the + constructor directly (except the factory itself), and fails if any alarm in the + synthesized template lacks actions. + +Use `expressionAlarm()` for metric math so the routing guarantee survives. + +## 2. No `config.production` in observability code + +This repo is forked by many institutions. A fork with one environment should not +have to reason about a `production` boolean, and a fork with three should not be +limited to two. Every tunable is a **single scalar** in `ObservabilityConfig`, +and per-environment differences live in the forker's own deployment config +(GitHub Variables scoped to a GitHub Environment) — not in a ternary here. + +Enforced: `observability-alarm-routing.test.ts` fails on any `config.production` +under `lib/constructs/observability/`. + +Adding a tunable means touching five places. Miss the third and the flag is +accepted then silently ignored: + +1. `OBSERVABILITY_DEFAULT_*` constant in `config.ts`, with the reasoning inline +2. field on `ObservabilityConfig` +3. loader entry using the full precedence chain (see §3) +4. `scripts/common/load-env.sh` → `build_cdk_context_params()` +5. job-level `env:` in `.github/workflows/platform.yml` + +Defaults are **cost-conscious**: they are what a fork inherits when it configures +nothing, so they are the cheapest setting that still leaves alerting useful. +Diagnostic depth is opt-in. The X-Ray sampling default is the clearest case — it +was `1.0` for any fork that never set `production`, meaning a recorded trace for +every single agent invocation at $5/million. + +## 3. The flat dotted context key (this has bitten the repo three times) + +`--context observability.logRetentionDays=90` sets the **flat** key +`context['observability.logRetentionDays']`. It does **not** build a nested +object. Reading only the nested form accepts the operator's flag and ignores it. + +```typescript +logRetentionDays: + parseIntEnv(process.env.CDK_OBSERVABILITY_LOG_RETENTION_DAYS) + ?? parseIntEnv(scope.node.tryGetContext('observability.logRetentionDays')) // ← flat + ?? scope.node.tryGetContext('observability')?.logRetentionDays // ← nested + ?? OBSERVABILITY_DEFAULT_LOG_RETENTION_DAYS, +``` + +Use `parseFloatEnv` for fractional values. `parseIntEnv('0.05')` is `0`, which +would switch X-Ray sampling off entirely rather than setting it to 5%. + +## 4. Gotchas that produce silently-broken alarms + +### The SNS topic must use a customer-managed KMS key + +CloudWatch **cannot** publish to a topic encrypted with the AWS-managed +`alias/aws/sns` key: the publish is made by the `cloudwatch.amazonaws.com` +service principal, and an AWS-managed key's policy cannot be edited to grant it +`kms:GenerateDataKey*`. The alarm goes to ALARM, the console shows it firing, and +the notification is dropped. + +`kms:Decrypt` alone is **not enough** — SNS envelope encryption has the +*publisher* generate the data key, so `GenerateDataKey*` is required too. + +### Dimensions must come from real resources + +A `CPUUtilization` alarm with no dimensions is a valid CloudWatch alarm that +silently averages every ECS service in the account. Same for ALB metrics. Always +derive dimensions from the CDK resource (`service.metricCpuUtilization()`, +`targetGroup.metrics.*`, `table.metric()`), never from a name string. + +### Metric-math alarms cap at 10 metrics + +CloudWatch rejects an alarm whose math expression contains more than 10 +individual metrics. CDK's `table.metricSystemErrorsForOperations()` defaults to +**all 14** DynamoDB operations and throws `TooManyMetricsInMathExpression` at +synth. Pass an explicit operations list. + +Also: CDK deprecates `metricThrottledRequests()` and `metricSystemErrors()` as +returning invalid metrics. Use `table.metric('ReadThrottleEvents')` etc. + +### Don't pass `label` to a metric used in an alarm + +It forces CDK to render the alarm as a `Metrics[]` array instead of flat +`Namespace`/`MetricName`/`ExtendedStatistic` properties. CloudWatch labels +percentile series adequately on its own. + +### Units differ between services + +| Metric | Unit | Threshold handling | +|---|---|---| +| AgentCore `Latency` | **Milliseconds** | use `agentCoreLatencyMs` directly | +| ALB `TargetResponseTime` | **Seconds** | divide by 1000 | + +Both verified with `get-metric-statistics`. Getting this wrong is a 1000x error +in either direction, and neither direction fails loudly. + +## 5. Streaming makes latency a weak signal + +The chat path is SSE. The ALB does not consider a request complete until the +stream closes, so `TargetResponseTime` and AgentCore `Latency` are legitimately +tens of seconds for a healthy turn — and a sudden *drop* can mean turns are +failing early. + +Measured over 14 days in dev: average turn **3.0–4.5s**, daily maxima **16.7s, +16.9s, 24.4s**. The original alarm threshold was 30s, i.e. *below* the observed +maximum, so a healthy long turn could trip it. Latency floors default to 120s. + +Reliable signals on this path are the discrete ones: 5xx counts, unhealthy hosts, +rejected connections, throttles. + +## 6. `treatMissingData` is a per-metric decision + +Never defaulted by the factory, because both answers are correct somewhere: + +- **`NOT_BREACHING`** for error and throttle counts. A service that is not + failing publishes nothing, so absent data is the healthy state. +- **`BREACHING`** for `UnHealthyHostCount` and `RunningTaskCount`. These stop + being published when the service is at zero or deleted. `NOT_BREACHING` would + leave the alarm silent during a total outage — the exact case it exists for. + +## 7. Verify metrics exist before alarming on them + +Documentation is not sufficient evidence that a metric is published, and an alarm +on a non-existent metric is indistinguishable from a healthy one. + +```bash +aws cloudwatch list-metrics --namespace "AWS/Bedrock-AgentCore" \ + | jq -r '.Metrics | group_by(.MetricName) | .[] | + "\(.[0].MetricName) dims=\(map(.Dimensions|map(.Name)|sort)|unique|tostring)"' +``` + +Known facts from that sweep, all pinned by tests: + +- Runtime metrics live in `AWS/Bedrock-AgentCore` (hyphenated), dimensioned + `Resource` + `Operation=InvokeAgentRuntime` + `Name={runtimeName}::DEFAULT`. + The lowercase `bedrock-agentcore` namespace is real but holds only the + OpenTelemetry/Strands *application* metrics. +- **Memory and Gateway publish `Resource` as a full ARN; Code Interpreter + publishes a bare ID.** Passing an ARN for Code Interpreter yields an alarm that + matches nothing. +- `AWS/Cognito` on the **ESSENTIALS** feature plan publishes *only* success + metrics. There is no sign-in failure or throttle metric to alarm on — failure + and threat metrics require the **Plus** plan. The auth-path failure signal is + the token-enrichment Lambda's `Errors` metric instead. +- AgentCore Browser has zero metric streams (provisioned, unused). + +A metric absent from `list-metrics` may simply never have fired. Alarming on it +is still correct — `NOT_BREACHING` keeps it quiet until the first occurrence. + +## 8. Resource budget + +CloudFormation caps a stack at **500 resources**, and this is a deliberate +single-stack architecture with nowhere to spill. Alarms are the largest +discretionary consumer. + +Measured: **308** resources before this work, **~370** after. A guard in +`observability-dynamodb-alarms.test.ts` fails the build above 460, so the ceiling +surfaces while there is still room to react rather than as a failed deploy. + +When budget matters, decide with data rather than by covering every documented +metric. The DynamoDB allocation was cut from 78 alarms to 27 because a live sweep +showed `ReadThrottleEvents`, `WriteThrottleEvents` and `SystemErrors` had **zero +streams** — all tables are on-demand, so throttling had never occurred — while +account-level `UserErrors` had real data nobody was watching. + +Dashboards: the first **3 are free**, then $3/month each. The stack is at exactly +3, which is why the platform dashboard links to the other two instead of +restating their widgets. + +## 9. Log retention + +One value, `observability.logRetentionDays`, applied through +`logRetentionFor(config)`. A source guard fails the build if any construct +hardcodes `retention: logs.RetentionDays.*`. + +The AgentCore Runtime's log group is created by the **AgentCore service**, not +CloudFormation, so a CDK `LogGroup` cannot set its retention — declaring one +would collide on create or manage a second, empty group. An `AwsCustomResource` +calls `logs:PutRetentionPolicy` instead. That API is idempotent *and* creates the +group if absent, which matters on a first deploy when the runtime exists but has +never been invoked. There is deliberately **no `onDelete`**: removing the +retention policy on teardown would revert the group to "keep forever", which is +the cost problem it fixes. + +## 10. Subscriptions are not infrastructure-as-code + +The topic is created by CDK; **subscribers are not**. Several teams need to hear +about failures and their membership changes far more often than the +infrastructure does. Requiring a PR, a review, and a CloudFormation deploy to add +one address is how a notification list goes stale and stops being trusted. + +```bash +TOPIC=$(aws ssm get-parameter --name "/${PREFIX}/observability/alarm-topic-arn" \ + --query Parameter.Value --output text) +aws sns subscribe --topic-arn "$TOPIC" \ + --protocol email --notification-endpoint team@example.edu +``` + +A test asserts zero `AWS::SNS::Subscription` resources exist, so this decision +cannot be quietly reversed. + +## 11. Runbook — first response by alarm + +| Alarm | What it means | First action | +|---|---|---| +| `alb-unhealthy-hosts` | Targets failing health checks, **or none reporting** | `aws ecs describe-services` — are tasks running at all? Then app-api logs. | +| `alb-elb-5xx` | The load balancer itself could not serve | Almost always no healthy target. Check the alarm above first. | +| `alb-target-5xx` | App is reachable and erroring | app-api logs. This is application code. | +| `alb-rejected-connections` | ALB connection limit hit | Users were turned away *before* reaching the app, so nothing is in app logs. Check request volume. | +| `app-api-running-tasks-low` | Fewer tasks than desired | Task failing to start: check stopped-task reason, image pull, subnet IPs. | +| `app-api-memory-high` | Sustained memory pressure | Fargate **kills** a task that exhausts memory. Raise memory or find the leak. | +| `agentcore-system-errors` | AgentCore's fault | Escalate to AWS. Not application code. | +| `agentcore-high-error-rate` | `UserErrors` — our requests are malformed | Recent inference-api deploy? Check payload shape and IAM. | +| `agentcore-throttles` | At the TPS or session quota | Request a quota increase. Will not self-resolve. | +| `agentcore-high-latency` | p99 above 120s | Genuinely hung, not merely slow — 24s is a normal maximum here. | +| `bedrock-tpm-quota-usage` | **Leading** indicator | Request a quota increase *now*, before throttling starts. | +| `bedrock-invocation-throttles` | At a model's TPM/RPM quota | Users see chats that never respond. Quota increase. | +| `agentcore-memory-*` | Memory hot path failing | Users experience an agent that has forgotten the conversation. | +| `agentcore-gateway-*` | MCP calls failing at the gateway | Agents lose tool access. Check gateway targets. | +| `ddb-*-throttle` | Named table throttling | On-demand, so this is a hot partition or an account limit. Compare `ReadThrottleEvents` vs `WriteThrottleEvents` on that table. | +| `ddb-user-errors` | DynamoDB rejecting our requests (4xx) | Application code misusing the API. Account-wide, so use CloudTrail or app logs to find the caller. | +| `lambda-token-enrichment-errors` | **Silent** degradation | Handler is fail-open: logins still work, but MCP tools are losing user-identity claims. | +| `dlq-kb-ingestion-not-empty` | Work accepted then failed every retry | Will **not** self-clear. Inspect, fix, then replay or drain. | +| `prompt-cache-session-partial-miss` | One conversation re-writing its prefix every turn | Use the "Sessions by partial-miss waste" widget to find which session. | + +Start at the **`{prefix}-platform-health`** dashboard: row 1 says whether traffic +is being served, row 2 says why, row 3 shows every alarm's current state. + +## 12. Boise State's own profile + +The committed defaults are what a **fork** should inherit. Boise State authors +this platform and does far more diagnostic work than any deployer of it, so our +values differ — and they live in GitHub Variables scoped to a GitHub Environment, +never in committed code. That separation is what lets both be right at once. + +`scripts/observability/set-bsu-overrides.sh --env ` +applies them. It is **not run by CI**, makes no AWS changes, and requires +confirmation, because it mutates shared repository configuration. Add `--dry-run` +to print the plan (no `gh` auth needed). + +| Field | OSS default | BSU dev | BSU prod | +|---|---|---|---| +| `xraySamplingRate` | `0.01` | `0.5` | `0.1` | +| `xraySamplingReservoir` | `1` | `5` | `2` | +| `xrayInsightsNotifications` | `false` | `true` | `true` | +| `agentCoreApplicationLogsEnabled` | `false` | `true` | **`false`** | +| `logRetentionDays` | `30` | `14` | `90` | +| `agentCoreErrorThreshold` | `10` | `5` | `5` | +| `lambdaErrorThreshold` | `5` | `1` | `3` | +| `albTarget5xxThreshold` | `10` | `5` | `5` | +| `dynamoThrottleThreshold` | `10` | `1` | default | +| `ecsCpuPercent` / `ecsMemoryPercent` | `80` / `85` | default | `75` / `80` | +| `promptCacheAvoidableMissThreshold` | `10` | `5` | default | +| `promptCacheWastedUsdThreshold` | `1` | `0.5` | default | + +Two choices worth understanding rather than copying: + +- **Dev traces at 50%, prod at 10%.** Not a mistake. X-Ray bills per trace + recorded, and prod traffic is orders of magnitude larger, so 10% of prod is far + more traces than 50% of dev. Dev is where we debug; prod is where we pay. +- **`agentCoreApplicationLogsEnabled` stays OFF in production.** Those records + carry every user's prompt and the model's response verbatim — the + highest-volume log source available and a genuine PII surface. Enable it + temporarily for a specific investigation, then turn it back off. + +Retention inverts between the two for the same reason latency floors are high: +dev keeps 14 days because dev noise is not worth a month, prod keeps 90 because a +real incident review reaches back weeks. + +**Verifying a variable took effect.** The synth log prints a line beginning +`Observability:` with the *resolved* values. If it disagrees with the variable you +set, the value is not reaching `--context` — check the deploy job's **job-level** +`env:` block, since `vars.*` in a workflow-level `env:` silently resolves to an +empty string. diff --git a/CHANGELOG.md b/CHANGELOG.md index 46f158e59..cab8e0de4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,78 @@ 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.17.0] - 2026-09-02 + +Reliability, security and observability. Every CloudWatch alarm in the stack now notifies somebody — before this the stack had 13 alarms and **none of them were routed**, two of which watched metric names that exist in no namespace and had read as healthy since the day they were created. A production outage post-mortem (session `5f34d2b0`) drives four chat-path changes: Bedrock's transient faults are retried, a retry and a long silence are both visible to the user, and attachments a failed turn never delivered are re-sent. Four security findings are closed, including a High-severity OIDC login CSRF in the BFF auth flow and a privilege-escalating stored XSS in skill resources. The Bedrock Managed Knowledge Base migration — still off by default — gets eleven defects fixed from its first real runs in dev. **Requires a CDK deploy**, and one manual step after it: subscribe your team to the new alarm topic (see [step-05-verify](.github/docs/deploy/step-05-verify.md#6-subscribe-to-platform-alarms-required--not-automated)). + +### 🚀 Added + +- **Single SNS alarm topic** (`{prefix}-alarms`) that every alarm routes to, encrypted with a customer-managed KMS key. The CMK is required, not a preference: CloudWatch cannot publish to a topic encrypted with the AWS-managed `alias/aws/sns` key, and that failure is silent — the alarm fires, the console shows it, the notification is dropped. Topic ARN published to SSM at `/{prefix}/observability/alarm-topic-arn` and as a CfnOutput (#910) +- **`AlarmFactory`** — the only sanctioned way to create an alarm. Attaches `AlarmActions` *and* `OKActions` as a consequence of being used, so an unrouted alarm now requires deliberately bypassing it. A source-level test fails the build if any file under `lib/` calls `new cloudwatch.Alarm()` directly (#910) +- **ALB alarms** (6): ELB 5xx, target 5xx, unhealthy hosts, target connection errors, rejected connections, and a streaming-aware p99 latency floor (#910) +- **ECS service alarms** (3): CPU, memory, and running-task-count below desired (#910) +- **DynamoDB alarms** (27): a combined read+write throttle alarm per table naming that table, plus one account-level `UserErrors` alarm (#910) +- **Lambda alarms** (21): errors and throttles across every runtime function, including `artifact-render`, `rag-ingestion` and the four kb-migration functions which previously had none, plus dead-letter-queue depth on the kb-ingestion DLQ (#910) +- **AI-path alarms** (9): Bedrock invocation throttles, server errors and `EstimatedTPMQuotaUsage` (the only *leading* indicator in the set — visible before throttling starts), AgentCore Memory hot-path errors and throttles, Gateway MCP errors and throttles, Code Interpreter session errors and concurrent-session count (#910) +- **`{prefix}-platform-health` dashboard** — one pane answering "is the platform healthy right now": traffic and errors, then saturation, then every alarm's current state. Links to the two existing dashboards rather than restating them, which keeps the stack at exactly 3 (CloudWatch's free ceiling) (#910) +- **`observability` configuration section** — 18 single-scalar tunables with `CDK_OBSERVABILITY_*` overrides, cost-conscious defaults, and validation that rejects retention values CloudWatch does not accept and X-Ray sampling rates given as percentages (#910) +- **`model_retry` SSE event** — the backend now tells the SPA when it is retrying a failed model call instead of surfacing it. The loading indicator swaps its cycling phrases for a fixed amber notice, cleared on `message_start`/`done` (#905) +- **Stall indicator** — the loading indicator says "Still working…" after 30s of silence and "Still working — this is taking longer than usual." after 90s, driven by a client-side liveness stamp on every stream event. A known retry outranks the stall notice (#907) +- **Attachment recovery** — a turn that dies before the model reads its inline documents now re-sends them on the next turn, via a write-ahead marker on the session row that survives every way a turn can die, including a dropped stream. Bounded to the single following turn, discarded after an hour, and the user's own attachments always win (#905) +- **`BedrockTransientRetryStrategy`** — `ServiceUnavailableException`, `InternalServerException`, `ModelNotReadyException` and `ModelTimeoutException` are now retried under the existing four-attempt backoff. Only when they fire *before* the response stream opens, so a retry can never replay visible output. `RETRY_TRANSIENT_SERVICE_ERRORS=false` restores stock behavior without a deploy (#905) +- `.kiro/steering/observability.md` — the gotchas that silently break alarms, plus a first-response runbook for every alarm (#910) +- `scripts/local-dev/run-kb-migration.py`, `kb-doc-timings.py` and `kb-compare-engines.py` — in-process drivers and read-only diagnostics for the managed-KB state machine (#898, #899) + +### ✨ Improved + +- A Bedrock 503 no longer reads as "I ran into a problem with the AI model". Two classifiers both keyed on `"throttl"` and neither recognized service-unavailable; a shared `is_service_unavailable_error` predicate now says the fault is on the provider's side and that we already retried (#905) + +### ⚠️ Changed + +- **Log retention is one configured value** (`observability.logRetentionDays`, default 30) applied to all 15 log groups through `logRetentionFor(config)`. Previously every construct hardcoded `ONE_WEEK`, except AgentCore Memory which used `ONE_MONTH` — differing silently rather than deliberately. A source guard fails the build on any hardcoded `RetentionDays` (#910) +- **No `config.production` branching in observability code.** This repo is forked by many institutions: a fork with one environment should not have to reason about a `production` boolean, and a fork with three should not be limited to two. Per-environment differences now live in the forker's deployment config as single values. Enforced by test (#910) +- **Skill resource files are served as `attachment`, never `inline`**, with their media type re-derived from the filename at serve time rather than reflected from the stored row — which is what neutralizes rows written before the upload allowlist existed, so no data migration is needed. Both SPA callers fetch these over XHR for an in-app viewer; nothing navigates to the URL (#904) +- **Managed KB no longer pins the embedding model.** `embeddingModelType: CUSTOM` and `rerankingModelType: MANAGED` are mutually exclusive — AWS rejects the combination — and measurement settled it: the pin scored 1.00/0.982/0.952 (flat, unusable for a small context cap) against managed reranking's 0.413/0.199. On managed retrieval Bedrock embeds both sides, so query/index consistency is its invariant, not ours. `embeddingModelId`/`embeddingDimensions` are no longer recorded, since nothing here knows what Bedrock chose (#898) + +### 🐛 Fixed + +- **A completed answer could tell the model it was cut short.** The client's Stop writes `lastTurnInterrupted` immediately, but the server only observes the armed cancel on a lease heartbeat that sleeps 10s *before* its first check — so a turn finishing inside that window completed normally and left the marker behind. The next turn then prepended a note saying the user stopped the previous response and not to resume it, every clause of which was false, and it demonstrably steered the answer. A turn that reaches the end of the success path now clears the marker, whatever the client signalled (#909) +- **A session id could be forked across two users.** `_get_session_by_gsi` returned `None` both for "no such session" and for "exists, but owned by someone else", so opening someone else's `/s/{sessionId}` link created a *second* metadata row under the requester — the `attribute_not_exists(PK)` guard cannot catch it, because the new row has a different PK. Not a confidentiality bug (conversation content lives in AgentCore Memory keyed by actor id, so the second user saw an empty thread), but it duplicated the row, mis-attached spend, and made the original owner's session resolve non-deterministically afterwards. Both GSI lookups now scan for the caller's own row rather than reading `items[0]`, since forked rows already exist (#906) +- **Bedrock's transient service faults were never retried.** Strands' stock retry strategy covers `ModelThrottledException` only, and `BedrockModel` maps exactly one error code to it — every other fault re-raised as a raw botocore `ClientError`, so the configured four-attempt backoff never ran. A prod `ConverseStream` carrying two PDFs failed with `ServiceUnavailableException` after 95.6s, billed 56,440 uncached input tokens, and returned nothing (#905) +- **The two AgentCore Runtime alarms were watching metrics that do not exist.** They used namespace `bedrock-agentcore` with `InvocationCount` / `InvocationErrors` / `InvocationLatency`. Verified against the live account: that namespace is real but holds only the OpenTelemetry/Strands *application* metrics, and those three names exist in no namespace at all. Corrected to `AWS/Bedrock-AgentCore` with the verified `Resource` + `Operation` + `Name` dimension set, and split into four alarms — `SystemErrors` (AWS's fault) separated from `UserErrors` (ours), plus a new throttle alarm (#910) +- **The AgentCore latency alarm would have fired on healthy traffic.** Its 30-second threshold sat *below* the observed maximum: measured over 14 days, average turns run 3.0–4.5s with daily maxima reaching 24.4s, because the chat path is SSE and the runtime does not finish a request until the stream closes. Floors now default to 120s (#910) +- **The `agentcore-observability` dashboard's token-usage widget was always empty** — `InputTokens`/`OutputTokens` do not exist, and the token metrics that do exist in that namespace are Memory-strategy counters dimensioned by `StrategyId`, not model tokens. Removed; the header now points at the prompt-cache dashboard for real token accounting (#910) +- **X-Ray recorded a trace for every single agent invocation** in any deployment that never set `production` — `fixedRate` was `1.0` with a 50/sec reservoir on that branch, at $5 per million traces recorded. Now a single configured value defaulting to 1% with a 1/sec reservoir (#910) +- **The AgentCore Runtime's log group had no retention policy and grew forever.** It is created by the AgentCore service rather than CloudFormation, so a CDK `LogGroup` cannot set it. An `AwsCustomResource` calls `logs:PutRetentionPolicy` instead — idempotent, and it creates the group if the runtime has not yet been invoked (#910) +- **Managed KB: the legacy pipeline never stood down for a promoted knowledge base.** Routing exclusivity was enforced on one side only, so every document added to a promoted KB was indexed twice — and worse, two writers owned one `status` field and the last one won by luck. Observed both ways in dev: a PDF marked `complete` 65s before the managed KB could answer for it, and an image-only PDF marked `failed` by Docling while Bedrock's image extraction had indexed and served it correctly. `handler.py` now resolves the engine before writing any status and returns early for `managed` (#900) +- **Managed KB: deleting a document never removed it from the managed knowledge base.** `cleanup_service` removed the legacy S3 Vectors copy and the `DOC#` row and left the managed copy indexed forever — paid for at $5.00/GB-month against S3 Vectors' ~$0.15, silently consuming `top_k` slots that the status filter then dropped, with the one fail-open branch of that filter left load-bearing. A third engine-gated phase is conjoined into `all_succeeded`, and unlike the ingestion gate an unreadable record **fails** rather than assuming legacy, because reporting success on a failed managed delete would remove the row *and* leave the content (#900) +- **Managed KB: provisioning could strand a knowledge base unrecoverably.** `CreateKnowledgeBase` returns while still `CREATING` (measured 47–124s to `ACTIVE`), and `CreateDataSource` was called immediately; `awsKbId` was only written after *both* creates succeeded, so a failure between them left a record with no identifier and every later attempt was refused permanently on the unique name. Now: an explicit bounded wait for `ACTIVE`, `awsKbId` persisted the moment the create returns (guarded on `attribute_not_exists`), and adopt-by-name recovery on a name-collision `ConflictException` — skipping terminal statuses, so adoption cannot take a knowledge base that is being deleted (#889, #898) +- **Managed KB: ingestion gave up before indexing finished, and re-ingested while it ran.** The consumer polled a *retrieval* for 30s — smaller than the documented lower bound for PDF ingestion — and, because `IngestKnowledgeBaseDocuments` is fire-and-forget, every EventBridge redelivery re-submitted the document and restarted the work it was waiting for. A 1.5MB PDF sat at `uploading` indefinitely with a fully retrievable copy in the KB. `handle_object` now probes `GetKnowledgeBaseDocuments` first and branches on the real `DocumentStatus` enum, with a 600s budget that a cross-language test asserts fits inside the Lambda timeout (#901) +- **Managed KB: `indexedAt` was fabricated** — set from the local clock the moment the ingest call returned, presented as when indexing finished, minutes apart for a large document. Now Bedrock's own `updatedAt` (#901) +- **Managed KB: `verify` failed good migrations for being asked too early.** The canary queried a freshly-ingested document before it was retrievable (measured ~45s on a fresh KB, against a warm 0.75–1.03s figure) and treated the empty result as terminal. Now defers via `records.defer_verify`, bounded by `MAX_VERIFY_ATTEMPTS` (#898) +- **Admin "Discover from server" returned 403 for every IAM-authenticated MCP server.** `POST /admin/tools/discover` signs with the app-api task role, which held `AddPermission`/`RemovePermission`/`GetFunctionUrlConfig` but never `lambda:InvokeFunctionUrl` — so admins had to type each tool name by hand in every environment (#911) + +### 🔒 Security + +- **OIDC login CSRF / session fixation in the BFF auth flow (High).** `GET /auth/login` minted a `state`, stored it server-side, and issued no browser-side material at all — no state cookie, no PKCE, no nonce — and `GET /auth/callback` treated "this state exists in the store" as proof the request continued a login *this* browser started. An attacker could mint a state anonymously, authenticate at the IdP themselves, and lure a victim to `/auth/callback?code=&state=`, silently issuing the victim a live session for the attacker's account (reported against a `system_admin` identity). Login now returns a 32-byte secret in a `__Host-bff_oauth_state` cookie and commits only its SHA-256 digest, checked with `secrets.compare_digest` **before** the state-store lookup so a probe cannot burn an in-flight state. PKCE (S256) and OIDC nonce verification are added end-to-end as defense in depth. A state row carrying no digest fails closed (#903) +- **Privilege-escalating stored XSS in skill resources.** A zero-privilege user could upload bytes labelled `text/html` — the routes persisted the client-supplied multipart Content-Type verbatim and permitted an `.html` filename — and the read routes reflected that type with `Content-Disposition: inline` while the CloudFront `/api/*` behavior carried no response-headers policy. Because app-api shares an origin with the SPA, the file parsed as a top-level HTML document and its inline `" +) + +# Every extension that turns a response body into a scriptable document. +ACTIVE_DOCUMENT_FILENAMES = [ + "vx.html", + "vx.htm", + "vx.xhtml", + "vx.xml", + "vx.svg", + "vx.shtml", + "vx.mhtml", + "vx.xht", +] + + +# --------------------------------------------------------------------------- +# Policy module — the allowlist itself +# --------------------------------------------------------------------------- + + +class TestResourceTypePolicy: + @pytest.mark.parametrize("filename", ACTIVE_DOCUMENT_FILENAMES) + def test_active_document_extensions_are_refused(self, filename): + with pytest.raises(SkillResourceTypeError): + resolve_upload_content_type(filename) + + def test_client_supplied_content_type_is_not_a_parameter(self): + """The signature itself is the control: there is nothing to spoof. + + The vulnerability was that the multipart header reached storage. If a + future refactor reintroduces a caller-supplied type argument, this fails. + """ + import inspect + + params = list(inspect.signature(resolve_upload_content_type).parameters) + assert params == ["filename"] + + def test_allowlist_contains_no_scriptable_media_type(self): + """No allowlist value may be a type a browser parses as a document.""" + forbidden = { + "text/html", + "application/xhtml+xml", + "image/svg+xml", + "text/xml", + "application/xml", + "text/javascript", + "application/javascript", + "application/x-shockwave-flash", + } + assert forbidden.isdisjoint(set(SAFE_EXTENSION_CONTENT_TYPES.values())) + + def test_code_extensions_store_as_inert_plain_text(self): + """D5: script resources are stored, listed, and never executable.""" + for ext in ("js", "mjs", "ts", "py", "sh", "css"): + assert resolve_upload_content_type(f"example.{ext}") == "text/plain" + + def test_ordinary_bundle_types_still_work(self): + assert resolve_upload_content_type("forms.md") == "text/markdown" + assert resolve_upload_content_type("data.json") == "application/json" + assert resolve_upload_content_type("chart.png") == "image/png" + assert resolve_upload_content_type("manual.pdf") == "application/pdf" + + def test_extension_match_is_case_insensitive(self): + """``.HTML`` must not slip past a lowercase-only comparison.""" + with pytest.raises(SkillResourceTypeError): + resolve_upload_content_type("vx.HTML") + assert resolve_upload_content_type("FORMS.MD") == "text/markdown" + + def test_double_extension_resolves_on_the_last_one(self): + """``notes.md.html`` is an HTML file, not a markdown file.""" + with pytest.raises(SkillResourceTypeError): + resolve_upload_content_type("notes.md.html") + + def test_extensionless_filename_is_refused(self): + with pytest.raises(SkillResourceTypeError): + resolve_upload_content_type("payload") + + def test_refusal_message_names_the_extension(self): + """An author who is blocked needs to know what to rename or convert.""" + with pytest.raises(SkillResourceTypeError, match=r"\.html"): + resolve_upload_content_type("vx.html") + + def test_download_type_never_reflects_an_active_type(self): + for filename in ACTIVE_DOCUMENT_FILENAMES: + assert safe_download_content_type(filename) == "application/octet-stream" + + def test_download_headers_are_attachment_nosniff_and_inert_csp(self): + headers = resource_download_headers("vx.html") + assert headers["Content-Disposition"].startswith("attachment;") + assert "inline" not in headers["Content-Disposition"] + assert headers["X-Content-Type-Options"] == "nosniff" + assert "default-src 'none'" in headers["Content-Security-Policy"] + assert "sandbox" in headers["Content-Security-Policy"] + + def test_download_headers_cannot_be_broken_out_of(self): + """A quote or CRLF in a legacy filename must not reach a header value.""" + headers = resource_download_headers('a".md\r\nX-Evil: 1') + value = headers["Content-Disposition"] + assert '"' not in value.removeprefix('attachment; filename="').removesuffix('"') + assert "\r" not in value and "\n" not in value + + +# --------------------------------------------------------------------------- +# Write side — the unprivileged upload route +# --------------------------------------------------------------------------- + + +@pytest.fixture() +def author_client(user_skill_service, author_user, monkeypatch): + """The attacker in the report: authenticated, zero admin privilege.""" + monkeypatch.setattr( + my_skill_routes, "get_user_skill_service", lambda: user_skill_service + ) + app = FastAPI() + app.include_router(my_skill_routes.router) + app.dependency_overrides[get_current_user_from_session] = lambda: author_user + with TestClient(app) as c: + yield c + + +@pytest.fixture() +def admin_client(skill_service, admin_user, monkeypatch): + """The victim in the report: an ``admin.skills`` holder reading a resource.""" + monkeypatch.setattr( + admin_skill_routes, "get_skill_catalog_service", lambda: skill_service + ) + app = FastAPI() + app.include_router(admin_skill_routes.router) + override_admin_auth(app, lambda: admin_user) + return TestClient(app) + + +def _create_my_skill(client, name="Verify XSS Skill QQ") -> str: + resp = client.post( + "/skills/mine", + json={"displayName": name, "description": "security verification"}, + ) + assert resp.status_code == 200, resp.text + return resp.json()["skillId"] + + +def _upload_mine(client, skill_id, filename, body, content_type, kind="reference"): + return client.post( + f"/skills/mine/{skill_id}/resources", + files={"file": (filename, body, content_type)}, + data={"kind": kind}, + ) + + +class TestUnprivilegedUploadIsAllowlisted: + def test_html_upload_is_rejected(self, author_client): + """Step 4 of the report: the upload that planted the payload.""" + skill_id = _create_my_skill(author_client) + resp = _upload_mine( + author_client, skill_id, "vx.html", XSS_PAYLOAD, "text/html" + ) + assert resp.status_code == 400, resp.text + assert "not allowed" in resp.json()["detail"].lower() + + # Nothing was persisted — not in the manifest, not in S3. + manifest = author_client.get(f"/skills/mine/{skill_id}/resources").json() + assert manifest["resources"] == [] + s3 = boto3.client("s3", region_name=AWS_REGION) + keys = [ + o["Key"] + for o in s3.list_objects_v2(Bucket=SKILL_RESOURCES_BUCKET).get( + "Contents", [] + ) + ] + assert not any(k.endswith("vx.html") for k in keys) + + @pytest.mark.parametrize("filename", ACTIVE_DOCUMENT_FILENAMES) + def test_every_active_document_extension_is_rejected( + self, author_client, filename + ): + skill_id = _create_my_skill(author_client) + resp = _upload_mine( + author_client, skill_id, filename, XSS_PAYLOAD, "text/html" + ) + assert resp.status_code == 400, f"{filename} was accepted" + + def test_html_payload_under_a_markdown_name_is_stored_as_markdown( + self, author_client + ): + """Bytes are not sniffed — the *served type* is what disarms them. + + ``.md`` is allowed and HTML bytes inside a markdown file are legitimate + (a documentation snippet), so this upload succeeds. What makes it inert + is that it is stored and served as ``text/markdown`` with ``attachment`` + + ``nosniff``, never as a document. + """ + skill_id = _create_my_skill(author_client) + resp = _upload_mine( + author_client, skill_id, "vx.md", XSS_PAYLOAD, "text/html" + ) + assert resp.status_code == 200, resp.text + assert resp.json()["resources"][0]["contentType"] == "text/markdown" + + def test_declared_content_type_is_ignored_entirely(self, author_client): + """The multipart header is attacker-controlled and must not be stored.""" + skill_id = _create_my_skill(author_client) + resp = _upload_mine( + author_client, skill_id, "notes.md", b"# notes", "text/html" + ) + assert resp.status_code == 200, resp.text + assert resp.json()["resources"][0]["contentType"] == "text/markdown" + + def test_script_kind_does_not_bypass_the_allowlist(self, author_client): + """``kind=script`` is about bundle layout, not about type freedom.""" + skill_id = _create_my_skill(author_client) + resp = _upload_mine( + author_client, + skill_id, + "vx.html", + XSS_PAYLOAD, + "text/html", + kind="script", + ) + assert resp.status_code == 400 + + +class TestAdminUploadIsAllowlisted: + """The admin catalog tier shares the code path and the same policy.""" + + def _create_catalog_skill(self, admin_client, skill_id="pdf_workflows"): + resp = admin_client.post( + "/skills/", + json={ + "skillId": skill_id, + "displayName": "PDF Workflows", + "description": "Fill, merge and split PDFs.", + "instructions": "# PDF Workflows", + }, + ) + assert resp.status_code == 200, resp.text + return skill_id + + def test_html_upload_is_rejected(self, admin_client): + skill_id = self._create_catalog_skill(admin_client) + resp = admin_client.post( + f"/skills/{skill_id}/resources", + files={"file": ("vx.html", XSS_PAYLOAD, "text/html")}, + ) + assert resp.status_code == 400, resp.text + + +# --------------------------------------------------------------------------- +# Read side — a legacy row must be neutralized without a data migration +# --------------------------------------------------------------------------- + + +def _plant_legacy_html_resource(service, skill_id, filename="vx.html"): + """Write a manifest entry the way the *vulnerable* upload path would have. + + Goes straight at the repository and the store, deliberately bypassing + ``add_resource``, because the point is to reproduce a row that already + exists in a deployed environment: ``content_type == "text/html"``. + """ + from apis.shared.skills.models import SkillResourceRef + from apis.shared.skills.resource_store import compute_content_hash + + key = service.resource_store.put( + skill_id=skill_id, + filename=filename, + content=XSS_PAYLOAD, + content_type="text/html", + kind="reference", + ) + ref = SkillResourceRef( + filename=filename, + content_hash=compute_content_hash(XSS_PAYLOAD), + size=len(XSS_PAYLOAD), + content_type="text/html", # the poisoned value + s3_key=key, + kind="reference", + ) + return ref + + +def _assert_response_cannot_execute(resp): + """The response body may be the payload; it may not be a document.""" + assert resp.status_code == 200, resp.text + assert resp.content == XSS_PAYLOAD # bytes are still readable by the SPA + assert "text/html" not in resp.headers["content-type"] + assert resp.headers["content-type"].startswith("application/octet-stream") + assert resp.headers["content-disposition"].startswith("attachment;") + assert "inline" not in resp.headers["content-disposition"] + assert resp.headers["x-content-type-options"] == "nosniff" + assert "default-src 'none'" in resp.headers["content-security-policy"] + + +class TestLegacyRowIsNeutralizedOnRead: + @pytest.mark.asyncio + async def test_admin_read_of_a_legacy_html_row( + self, admin_client, skill_service, admin_user + ): + """Step 5 of the report: the request that executed the script.""" + admin_client.post( + "/skills/", + json={ + "skillId": "legacy_skill", + "displayName": "Legacy", + "description": "d", + "instructions": "i", + }, + ) + ref = _plant_legacy_html_resource(skill_service, "legacy_skill") + await skill_service.repository.update_skill( + "legacy_skill", {"resources": [ref]}, admin_user_id=admin_user.user_id + ) + + resp = admin_client.get("/skills/legacy_skill/resources/vx.html") + _assert_response_cannot_execute(resp) + + @pytest.mark.asyncio + async def test_owner_read_of_a_legacy_html_row( + self, author_client, user_skill_service, author_user + ): + """The owner-tier read route is hardened identically.""" + skill_id = _create_my_skill(author_client, "Legacy Mine") + ref = _plant_legacy_html_resource( + user_skill_service.catalog_service, skill_id + ) + await user_skill_service.repository.update_skill( + skill_id, {"resources": [ref]}, admin_user_id=author_user.user_id + ) + + resp = author_client.get(f"/skills/mine/{skill_id}/resources/vx.html") + _assert_response_cannot_execute(resp) + + +class TestHardenedHeadersOnOrdinaryReads: + def test_markdown_read_keeps_its_type_but_gains_the_headers( + self, author_client + ): + """The SPA reads these over XHR as text, so ``attachment`` is safe.""" + skill_id = _create_my_skill(author_client, "Notes Skill") + _upload_mine(author_client, skill_id, "forms.md", b"# Forms", "text/markdown") + + resp = author_client.get(f"/skills/mine/{skill_id}/resources/forms.md") + assert resp.status_code == 200 + assert resp.content == b"# Forms" + assert resp.headers["content-type"].startswith("text/markdown") + assert resp.headers["content-disposition"].startswith("attachment;") + assert resp.headers["x-content-type-options"] == "nosniff" + + def test_no_read_route_serves_inline(self): + """Pins the header both tiers regressed on, at the source level.""" + import inspect + + for module in (my_skill_routes, admin_skill_routes): + source = inspect.getsource(module) + assert "inline; filename=" not in source, module.__name__ diff --git a/backend/tests/apis/app_api/skills/test_user_skill_service.py b/backend/tests/apis/app_api/skills/test_user_skill_service.py index 509648bb8..b42b21045 100644 --- a/backend/tests/apis/app_api/skills/test_user_skill_service.py +++ b/backend/tests/apis/app_api/skills/test_user_skill_service.py @@ -365,12 +365,17 @@ async def test_admin_catalog_list_excludes_user_skills( async def test_user_skills_cannot_be_granted_to_app_roles( user_skill_service, skill_service, admin_user, author_user ): - """Granting a private user skill to a role would leak it to that role.""" + """Granting a private user skill to a role would leak it to that role. + + The refusal reports "not found" rather than "user-authored": the admin + catalog surface must not confirm the existence of a skill it cannot + govern, so a cross-tier id is indistinguishable from an unknown one. + """ mine = await user_skill_service.create_my_skill( author_user, display_name="Mine", description="d" ) - with pytest.raises(ValueError, match="user-authored"): + with pytest.raises(ValueError, match="not found"): await skill_service.set_roles_for_skill(mine.skill_id, ["some_role"], admin_user) - with pytest.raises(ValueError, match="user-authored"): + with pytest.raises(ValueError, match="not found"): await skill_service.add_roles_to_skill(mine.skill_id, ["some_role"], admin_user) diff --git a/backend/tests/apis/inference_api/test_attachment_recovery.py b/backend/tests/apis/inference_api/test_attachment_recovery.py new file mode 100644 index 000000000..24027c76e --- /dev/null +++ b/backend/tests/apis/inference_api/test_attachment_recovery.py @@ -0,0 +1,61 @@ +"""Recovery of attachments a failed turn consumed without ever being read. + +Inline document bytes are one-shot: `TurnBasedSessionManager._strip_document_bytes` +removes them from restored history (Bedrock rejects duplicate document names), +so a turn that dies before the model reads the documents loses them for good. + +Prod session `5f34d2b0` (2026-08-31) is the incident this covers — a +ConverseStream carrying two PDFs failed with ServiceUnavailableException, the +PDFs were gone, and the model's only recourse was to ask the user to upload +them again. The invocations route now pops a write-ahead marker at turn start +and re-sends those uploads on the very next turn. +""" + +from apis.inference_api.chat.routes import ( + _build_attachment_recovery_note, + _select_recovered_attachments, +) + + +class TestSelectRecoveredAttachments: + def test_re_attaches_when_the_turn_has_none_of_its_own(self): + assert _select_recovered_attachments(["up-1", "up-2"]) == ["up-1", "up-2"] + + def test_nothing_recovered_is_a_noop(self): + assert _select_recovered_attachments([]) == [] + + def test_user_upload_ids_win(self): + """Merging could push a deliberately-attached file past the 5-file + resolver cap, and re-sending the same document twice is a Bedrock + ValidationException.""" + assert _select_recovered_attachments(["up-1"], request_upload_ids=["fresh"]) == [] + + def test_direct_file_content_wins(self): + assert _select_recovered_attachments(["up-1"], request_files=[object()]) == [] + + def test_empty_request_collections_do_not_count_as_attachments(self): + assert _select_recovered_attachments( + ["up-1"], request_upload_ids=[], request_files=[] + ) == ["up-1"] + + def test_returns_a_copy(self): + original = ["up-1"] + result = _select_recovered_attachments(original) + result.append("up-2") + assert original == ["up-1"] + + +class TestBuildAttachmentRecoveryNote: + def test_names_the_files_and_forbids_asking_for_a_re_upload(self): + """The whole failure mode was the model telling the user to re-upload + files the server already had.""" + note = _build_attachment_recovery_note(["a.pdf", "b.pdf"]) + assert note.startswith("") + assert note.endswith("") + assert "a.pdf" in note and "b.pdf" in note + assert "do not ask the user to upload them" in note + + def test_says_the_user_did_not_re_attach_them(self): + note = _build_attachment_recovery_note(["report.pdf"]) + assert "did not attach them again" in note + assert "previous turn" in note diff --git a/backend/tests/documents/test_managed_delete_propagation.py b/backend/tests/documents/test_managed_delete_propagation.py new file mode 100644 index 000000000..8adaacb6a --- /dev/null +++ b/backend/tests/documents/test_managed_delete_propagation.py @@ -0,0 +1,222 @@ +"""Deleting a document must remove it from the engine that actually serves it. + +Before this, `cleanup_service` deleted the legacy S3 Vectors copy and the `DOC#` +row and never touched the managed knowledge base. On a promoted knowledge base +the content therefore stayed indexed forever: + +* it kept being paid for, at $5.00/GB-month against S3 Vectors' ~$0.15; +* every orphan kept consuming a slot in `top_k`, because the status filter runs + *after* retrieval — so a query could return five chunks and the model see two, + with nothing logged and nothing raised; +* and the only thing preventing deleted content from being served was the + fail-closed status filter, which has exactly one fail-open branch. + +The most load-bearing assertion in this file is +`test_a_failed_managed_delete_keeps_the_document_record`: the `DOC#` row is what +the status filter joins against, so reporting success on a failed managed delete +would remove the row *and* leave the content — turning a storage leak into a +disclosure. +""" + +from __future__ import annotations + +from typing import Any, Dict, List, Optional + +import pytest + +from apis.app_api.documents.services import cleanup_service + +ASSISTANT_ID = "ast-del-1" +DOCUMENT_ID = "DOC-del-1" + + +class RecordingManagedBackend: + """Stands in for ManagedKbBackend, recording deletes and optionally failing.""" + + def __init__(self, raises: Optional[BaseException] = None) -> None: + self.deleted: List[str] = [] + self.attempts = 0 + self._raises = raises + + async def delete_document(self, kb_ref: str, document_id: str) -> None: + self.attempts += 1 + if self._raises is not None: + raise self._raises + self.deleted.append(document_id) + + +@pytest.fixture +def managed(monkeypatch: pytest.MonkeyPatch) -> RecordingManagedBackend: + backend = RecordingManagedBackend() + import apis.shared.kb_backend.managed_backend as mb + + monkeypatch.setattr(mb, "ManagedKbBackend", lambda *a, **k: backend) + return backend + + +def _set_engine(monkeypatch: pytest.MonkeyPatch, engine: Optional[str]) -> None: + from apis.shared.kb_backend import records as r + + record: Optional[Dict[str, Any]] + if engine is None: + record = None + else: + record = {"retrievalEngine": engine} if engine != "absent" else {} + + monkeypatch.setattr(r, "get_kb_record", lambda *_: record) + + +def _raise_on_lookup(monkeypatch: pytest.MonkeyPatch, exc: BaseException) -> None: + from apis.shared.kb_backend import records as r + + def _boom(*_: Any): + raise exc + + monkeypatch.setattr(r, "get_kb_record", _boom) + + +async def _delete() -> bool: + return await cleanup_service._delete_managed_documents_with_retries( + ASSISTANT_ID, DOCUMENT_ID, max_retries=3, base_delay=0.0 + ) + + +class TestAPromotedKnowledgeBaseHasTheDocumentRemoved: + @pytest.mark.asyncio + async def test_the_document_is_deleted_from_the_managed_kb( + self, managed: RecordingManagedBackend, monkeypatch + ): + _set_engine(monkeypatch, "managed") + + assert await _delete() is True + assert managed.deleted == [DOCUMENT_ID], ( + "a document deleted by its owner was left in the managed knowledge " + "base; the corpus can only grow and every orphan costs a top_k slot" + ) + + +class TestALegacyKnowledgeBaseIsUntouched: + """No managed copy to remove is success, not failure. + + Returning False here would block `hard_delete_document` for every legacy + document in the system — the deletion would never complete. + """ + + @pytest.mark.asyncio + async def test_an_absent_record_needs_no_managed_delete( + self, managed: RecordingManagedBackend, monkeypatch + ): + _set_engine(monkeypatch, None) + + assert await _delete() is True + assert managed.deleted == [] + + @pytest.mark.asyncio + async def test_a_record_with_no_engine_needs_no_managed_delete( + self, managed: RecordingManagedBackend, monkeypatch + ): + """A migration in flight: `shadow`/`verify` have not promoted anything.""" + _set_engine(monkeypatch, "absent") + + assert await _delete() is True + assert managed.deleted == [] + + +class TestFailuresKeepTheDocumentRecordAlive: + """The `DOC#` row is the safety net, so a failure must not report success.""" + + @pytest.mark.asyncio + async def test_a_failed_managed_delete_keeps_the_document_record( + self, monkeypatch + ): + backend = RecordingManagedBackend(raises=RuntimeError("AccessDenied")) + import apis.shared.kb_backend.managed_backend as mb + + monkeypatch.setattr(mb, "ManagedKbBackend", lambda *a, **k: backend) + _set_engine(monkeypatch, "managed") + + assert await _delete() is False, ( + "a failed managed delete reported success; the caller would then " + "hard-delete the DOC# row that the fail-closed status filter joins " + "against, leaving indexed content with nothing left to hide it" + ) + assert backend.attempts == 3, "every attempt should be retried" + + @pytest.mark.asyncio + async def test_an_unreadable_record_fails_rather_than_assuming_legacy( + self, managed: RecordingManagedBackend, monkeypatch + ): + """The opposite choice from the ingestion gate, on purpose. + + On ingest an unreadable record resolves to legacy, because the cost of + being wrong is a duplicate index while the consumer still finishes the + document. On delete the cost of being wrong is hard-deleting the row that + keeps a still-indexed chunk unserved, so it fails and retries instead. + """ + _raise_on_lookup(monkeypatch, RuntimeError("DynamoDB unavailable")) + + assert await _delete() is False + assert managed.deleted == [] + + @pytest.mark.asyncio + async def test_a_promoted_kb_with_no_aws_ids_is_not_retried_forever( + self, monkeypatch + ): + """Nothing was ever indexed, so there is nothing to remove.""" + from apis.shared.kb_backend.managed_backend import ManagedKbNotProvisioned + + backend = RecordingManagedBackend(raises=ManagedKbNotProvisioned("no awsKbId")) + import apis.shared.kb_backend.managed_backend as mb + + monkeypatch.setattr(mb, "ManagedKbBackend", lambda *a, **k: backend) + _set_engine(monkeypatch, "managed") + + assert await _delete() is True + assert backend.attempts == 1, "not provisioned is terminal, not transient" + + +class TestTheCleanupContractIncludesManagedDeletion: + @pytest.mark.asyncio + async def test_a_failed_managed_delete_blocks_the_hard_delete( + self, monkeypatch + ): + """End to end through `cleanup_document_resources`, not just the helper. + + The helper returning False is only useful if the caller conjoins it. This + is the assertion that would fail if someone computed `all_succeeded` + without the managed phase. + """ + hard_deleted: List[str] = [] + + async def _no_hard_delete(assistant_id: str, document_id: str) -> None: + hard_deleted.append(document_id) + + async def _ok(*_: Any, **__: Any) -> bool: + return True + + monkeypatch.setattr(cleanup_service, "_delete_vectors_with_retries", _ok) + monkeypatch.setattr(cleanup_service, "_delete_s3_with_retries", _ok) + + async def _managed_fails(*_: Any, **__: Any) -> bool: + return False + + monkeypatch.setattr( + cleanup_service, "_delete_managed_documents_with_retries", _managed_fails + ) + + import apis.app_api.documents.services.document_service as ds + + monkeypatch.setattr(ds, "hard_delete_document", _no_hard_delete) + + result = await cleanup_service.cleanup_document_resources( + document_id=DOCUMENT_ID, + assistant_id=ASSISTANT_ID, + s3_key=f"assistants/{ASSISTANT_ID}/documents/{DOCUMENT_ID}/f.pdf", + chunk_count=3, + ) + + assert result is False + assert hard_deleted == [], ( + "the DOC# row was hard-deleted even though the managed knowledge " + "base still holds the content" + ) diff --git a/backend/tests/ingestion/test_ingestion_engine_gate.py b/backend/tests/ingestion/test_ingestion_engine_gate.py new file mode 100644 index 000000000..65715f2a5 --- /dev/null +++ b/backend/tests/ingestion/test_ingestion_engine_gate.py @@ -0,0 +1,252 @@ +"""The legacy ingestion pipeline must not touch a promoted knowledge base. + +Routing exclusivity (design §537, Requirement 10.5). Both pipelines are triggered +by the same S3 upload — the legacy `s3:ObjectCreated` notification and the managed +consumer's EventBridge rule — so exactly one of them has to stand down per +document. The consumer already returns immediately for a legacy document; these +tests cover the half that was missing, which is this pipeline standing down for a +*managed* one. + +WHY THE STATUS HALF MATTERS MORE THAN THE DUPLICATE VECTORS +Two writers owning one `status` field means the last writer wins by luck. Both +outcomes were observed in dev before this gate existed: + +* A PDF was marked `complete` by this pipeline 65 s before the managed knowledge + base could answer for it — "your document is ready", then an answer that does + not mention it. +* An image-only PDF that Docling could not parse at all (`Docling produced zero + chunks`) was marked `failed` while the managed knowledge base was serving it + correctly. That one only read `complete` in the end because the managed + consumer happened to finish second and overwrite it. Reverse the finishing + order — entirely a matter of document size and parse time — and a good, + retrievable document reads `failed` forever, with no way to retry it. + +So the assertions below are mostly about what is *not* written. +""" + +from __future__ import annotations + +import json +import sys +import types +from typing import Any, Dict, List, Optional + +import pytest + +from apis.app_api.documents.ingestion import handler as handler_module + +ASSISTANT_ID = "ast-gate-1" +DOCUMENT_ID = "DOC-gate-1" + + +class RecordingStatusManager: + """Captures every status transition the handler attempts.""" + + def __init__(self) -> None: + self.calls: List[str] = [] + + async def mark_chunking(self, **_: Any) -> None: + self.calls.append("chunking") + + async def mark_embedding(self, **_: Any) -> None: + self.calls.append("embedding") + + async def mark_complete(self, **_: Any) -> None: + self.calls.append("complete") + + async def mark_failed(self, **_: Any) -> None: + self.calls.append("failed") + + +@pytest.fixture +def status_manager(monkeypatch: pytest.MonkeyPatch) -> RecordingStatusManager: + """Stand in for the `status` module, which only resolves inside the image. + + `handler.py` does `from status import create_status_manager` as a bare + top-level import because the Dockerfile flattens `documents/ingestion/` onto + LAMBDA_TASK_ROOT. Injecting the module is how a test calls the handler + without reproducing that layout. + """ + recorder = RecordingStatusManager() + fake = types.ModuleType("status") + fake.create_status_manager = lambda: recorder # type: ignore[attr-defined] + monkeypatch.setitem(sys.modules, "status", fake) + return recorder + + +@pytest.fixture +def no_real_pipeline(monkeypatch: pytest.MonkeyPatch) -> List[str]: + """Replace the Docling/embedding pipeline; record whether it was reached.""" + reached: List[str] = [] + + async def _fake_pipeline(**kwargs: Any) -> None: + reached.append(kwargs.get("document_id", "?")) + + monkeypatch.setattr(handler_module, "_process_document_pipeline", _fake_pipeline) + return reached + + +def _kb_record(engine: Optional[str]) -> Dict[str, Any]: + record: Dict[str, Any] = {"PK": f"AST#{ASSISTANT_ID}", "SK": f"KB#{ASSISTANT_ID}"} + if engine is not None: + record["retrievalEngine"] = engine + return record + + +def _set_record(monkeypatch: pytest.MonkeyPatch, record) -> None: + """Point `records.get_kb_record` at a fixed answer, or make it raise.""" + from apis.shared.kb_backend import records as r + + def _get(_assistant_id: str, _app_kb_id: str): + if isinstance(record, Exception): + raise record + return record + + monkeypatch.setattr(r, "get_kb_record", _get) + + +def _event() -> Dict[str, Any]: + return { + "Records": [ + { + "s3": { + "bucket": {"name": "docs-bucket"}, + "object": { + "key": ( + f"assistants/{ASSISTANT_ID}/documents/" + f"{DOCUMENT_ID}/flowchart.pdf" + ) + }, + } + } + ] + } + + +async def _invoke() -> Dict[str, Any]: + return await handler_module.async_lambda_handler(_event(), None) + + +class TestAPromotedKnowledgeBaseIsLeftAlone: + @pytest.mark.asyncio + async def test_no_document_status_is_written_at_all( + self, status_manager: RecordingStatusManager, no_real_pipeline, monkeypatch + ): + """The whole point. `complete` and `failed` both belong to the consumer.""" + _set_record(monkeypatch, _kb_record("managed")) + + response = await _invoke() + + assert status_manager.calls == [], ( + "the legacy pipeline wrote document status for a knowledge base served " + "by the managed engine; two writers on one field is how a good " + "document ends up reading 'failed'" + ) + assert response["statusCode"] == 200 + assert "Skipped" in json.loads(response["body"])["message"] + + @pytest.mark.asyncio + async def test_the_document_is_not_parsed_or_embedded( + self, status_manager, no_real_pipeline: List[str], monkeypatch + ): + """No Docling time and no S3 Vectors storage for vectors nothing reads.""" + _set_record(monkeypatch, _kb_record("managed")) + + await _invoke() + + assert no_real_pipeline == [] + + @pytest.mark.asyncio + async def test_it_returns_success_so_the_event_is_not_retried( + self, status_manager, no_real_pipeline, monkeypatch + ): + """A skip is a correct outcome, not a failure to redeliver.""" + _set_record(monkeypatch, _kb_record("managed")) + + response = await _invoke() + + assert response["statusCode"] == 200 + assert json.loads(response["body"])["engine"] == "managed" + + +class TestEveryOtherKnowledgeBaseStillRuns: + """The gate keys on the ENGINE, not on the presence of a record. + + During `shadow` and `verify` the legacy path is still authoritative and must + keep working (Requirements 16.1, 16.6). Only `promote` writes + `retrievalEngine`, so a record that exists but names no engine — which is + exactly a migration in flight — must not be skipped. + """ + + @pytest.mark.asyncio + async def test_a_record_with_no_engine_still_runs( + self, status_manager: RecordingStatusManager, no_real_pipeline: List[str], monkeypatch + ): + _set_record(monkeypatch, _kb_record(None)) + + await _invoke() + + assert status_manager.calls == ["chunking"] + assert no_real_pipeline == [DOCUMENT_ID] + + @pytest.mark.asyncio + async def test_a_migration_in_flight_still_runs( + self, status_manager: RecordingStatusManager, no_real_pipeline: List[str], monkeypatch + ): + """`shadow` is not `promoted`. Skipping here would strand live uploads.""" + record = _kb_record(None) + record["migrationState"] = "shadow" + _set_record(monkeypatch, record) + + await _invoke() + + assert status_manager.calls == ["chunking"] + assert no_real_pipeline == [DOCUMENT_ID] + + @pytest.mark.asyncio + async def test_no_record_at_all_still_runs( + self, status_manager: RecordingStatusManager, no_real_pipeline: List[str], monkeypatch + ): + """Every knowledge base that predates this feature. Absence ⇒ legacy.""" + _set_record(monkeypatch, None) + + await _invoke() + + assert status_manager.calls == ["chunking"] + assert no_real_pipeline == [DOCUMENT_ID] + + +class TestAnUnreadableRecordRunsTheLegacyPipeline: + """Fail towards legacy, deliberately — the two errors are not symmetric. + + Wrong towards legacy on a promoted knowledge base costs a duplicate index and + a status race, and the managed consumer still drives the document to a correct + terminal state. Wrong towards skipping on a legacy knowledge base means + nothing indexes the document at all: it sits un-ingested with no error, and + the only way out is a re-upload. The second is much worse. + """ + + @pytest.mark.asyncio + async def test_a_lookup_failure_does_not_skip_the_document( + self, status_manager: RecordingStatusManager, no_real_pipeline: List[str], monkeypatch + ): + _set_record(monkeypatch, RuntimeError("DynamoDB unavailable")) + + await _invoke() + + assert status_manager.calls == ["chunking"], ( + "an unreadable KB record skipped the document; a transient read " + "failure must not silently leave an upload un-ingested" + ) + assert no_real_pipeline == [DOCUMENT_ID] + + @pytest.mark.asyncio + async def test_a_lookup_failure_is_not_reported_as_a_document_failure( + self, status_manager: RecordingStatusManager, no_real_pipeline, monkeypatch + ): + """The user's document is fine; our read of an unrelated row was not.""" + _set_record(monkeypatch, RuntimeError("DynamoDB unavailable")) + + await _invoke() + + assert "failed" not in status_manager.calls diff --git a/backend/tests/lambdas/test_kb_ingestion_consumer.py b/backend/tests/lambdas/test_kb_ingestion_consumer.py index 70d20246b..cf0d07fed 100644 --- a/backend/tests/lambdas/test_kb_ingestion_consumer.py +++ b/backend/tests/lambdas/test_kb_ingestion_consumer.py @@ -13,6 +13,7 @@ legacy must ingest NOTHING here, managed must ingest here and NOT fall back. """ +from datetime import datetime, timezone from unittest.mock import MagicMock, patch import boto3 @@ -79,12 +80,67 @@ def _eventbridge_event(key=KEY): return {"detail": {"bucket": {"name": BUCKET}, "object": {"key": key}}} +class _FakeAgentClient: + """The control-plane surface the consumer reads document status from.""" + + def __init__(self, owner): + self._owner = owner + + def get_knowledge_base_documents(self, **kwargs): + self._owner.status_calls += 1 + status = self._owner.next_status() + if status == "NOT_FOUND": + return {"documentDetails": []} + return { + "documentDetails": [ + { + "knowledgeBaseId": "KB123", + "dataSourceId": "DS456", + "status": status, + "identifier": { + "dataSourceType": "CUSTOM", + "custom": {"id": DOCUMENT_ID}, + }, + "updatedAt": datetime(2026, 9, 1, 14, 53, 19, tzinfo=timezone.utc), + } + ] + } + + class _FakeBackend: - """Records ingest calls; reports the document retrievable immediately.""" + """Models the parts of ManagedKbBackend the consumer actually leans on. - def __init__(self): + Deliberately models Bedrock's document STATUS, not just the ingest call. + Ingestion is fire-and-forget: the API returns once the request is accepted and + says nothing about progress, so a fake that only recorded ingests could not + express the state the consumer now has to reason about — and a fake that + reported instant success is what let the 30 s poll window look adequate for + documents that take minutes. + + ``statuses`` is the sequence returned by successive status probes. The default + models a small document: unknown, then indexed. Pass a longer sequence to model + a slow one; the last value repeats forever. + """ + + def __init__(self, statuses=None): self.ingested = [] + self.status_calls = 0 + self._statuses = list(statuses or ["NOT_FOUND", "INDEXED"]) + self._agent_client = _FakeAgentClient(self) + + def next_status(self): + if len(self._statuses) > 1: + return self._statuses.pop(0) + return self._statuses[0] + # -- the private surface `document_status` reuses -------------------------- + def _agent(self): + return self._agent_client + + def _locate(self, kb_ref): + return ("KB123", "DS456") + + # -- the protocol surface -------------------------------------------------- async def ingest(self, kb_ref, source): self.ingested.append(source.document_id) return None @@ -211,6 +267,31 @@ def test_indexed_and_retrievable_are_recorded_separately(self, table): assert "retrievableAt" in item assert result["indexedAt"] and result["retrievableAt"] + def test_indexed_at_is_bedrocks_timestamp_not_our_clock(self, table): + """`indexedAt` must be the value Bedrock reports, not the local time. + + The original code set `indexed_at = _now_iso()` immediately after the + ingest call returned. That call is fire-and-forget — it returns when the + request is accepted — so the field recorded "when we asked", presented as + "when indexing finished". For a 1.5 MB PDF in dev those were 5.5 minutes + apart, and because the field was always populated the error was invisible: + the pre-existing test asserted only that the key existed and was truthy, + which a fabricated value satisfies perfectly. + """ + self._seed_managed(table) + with patch( + "apis.shared.kb_backend.managed_backend.ManagedKbBackend", + return_value=_FakeBackend(), + ): + result = ic.handle_object(BUCKET, KEY) + + # The fake reports 2026-09-01T14:53:19+00:00 from GetKnowledgeBaseDocuments. + assert result["indexedAt"].startswith("2026-09-01T14:53:19"), ( + f"indexedAt is {result['indexedAt']!r}, which is not the timestamp " + f"Bedrock reported — it looks like a local clock reading" + ) + assert _doc(table)["indexedAt"].startswith("2026-09-01T14:53:19") + def test_a_managed_document_never_falls_back_to_legacy(self, table): """Managed engine but unprovisioned must FAIL, not silently degrade. @@ -330,3 +411,152 @@ def test_the_module_does_not_use_ensure_future(self): } assert "ensure_future" not in called assert "create_task" not in called + + +# --------------------------------------------------------------------------- +# A slow document must converge, not die +# --------------------------------------------------------------------------- +class TestSlowIndexingConverges: + """The defect a 1.5 MB PDF exposed in dev on 2026-09-01. + + Ingestion succeeded, but the consumer polled a *retrieval* for 30 s starting + the instant the ingest call returned — before Bedrock had indexed anything. + That poll window was sized against the INDEXED -> retrievable gap + (0.75-1.03 s), while it actually had to cover ingest -> INDEXED -> retrievable, + which the §5.1 benchmark measured at 37-264 s for PDFs. + + Each of the three redeliveries then RE-INGESTED, discarding progress and + restarting the clock. The document reached INDEXED 54 s after the final attempt + was dead-lettered, leaving a fully retrievable document parked at `uploading` + with nothing left to reconcile it — the legacy pipeline no longer writes status + for a promoted knowledge base, so there was no second writer to mask it. + """ + + def _seed_managed(self, table): + _seed_kb(table, retrievalEngine="managed", awsKbId="KB123", awsDataSourceId="DS456") + + def test_a_document_already_being_indexed_is_not_re_ingested(self, table): + """The core fix. Re-submitting restarts the work we are waiting for.""" + self._seed_managed(table) + fake = _FakeBackend(statuses=["IN_PROGRESS"]) + + with patch( + "apis.shared.kb_backend.managed_backend.ManagedKbBackend", return_value=fake + ): + with pytest.raises(ic.IngestionRoutingError, match="IN_PROGRESS"): + ic.handle_object(BUCKET, KEY) + + assert fake.ingested == [], ( + "a document Bedrock was already indexing was submitted again; that " + "discards the progress this invocation is waiting on" + ) + + def test_a_document_still_indexing_is_left_non_terminal(self, table): + """Not complete and not failed — the next delivery decides.""" + self._seed_managed(table) + fake = _FakeBackend(statuses=["IN_PROGRESS"]) + + with patch( + "apis.shared.kb_backend.managed_backend.ManagedKbBackend", return_value=fake + ): + with pytest.raises(ic.IngestionRoutingError): + ic.handle_object(BUCKET, KEY) + + item = _doc(table) + assert item["status"] not in ("complete", "failed") + assert "indexedAt" not in item, "no timestamp may be invented before indexing" + + def test_a_redelivery_completes_the_document_without_a_second_ingest(self, table): + """Delivery 1 submits and defers; delivery 2 finds it INDEXED and finishes. + + This is the whole convergence property: the document ends up `complete` + having been handed to Bedrock exactly once. + """ + self._seed_managed(table) + + # Delivery 1: never seen, then still working for the whole in-invocation wait. + first = _FakeBackend(statuses=["NOT_FOUND", "IN_PROGRESS"]) + with patch( + "apis.shared.kb_backend.managed_backend.ManagedKbBackend", return_value=first + ), patch.object(ic, "INDEXED_POLL_TIMEOUT_SECONDS", 0.01): + with pytest.raises(ic.IngestionRoutingError): + ic.handle_object(BUCKET, KEY) + assert first.ingested == [DOCUMENT_ID] + assert _doc(table)["status"] not in ("complete", "failed") + + # Delivery 2: Bedrock has finished. + second = _FakeBackend(statuses=["INDEXED"]) + with patch( + "apis.shared.kb_backend.managed_backend.ManagedKbBackend", return_value=second + ): + ic.handle_object(BUCKET, KEY) + + assert second.ingested == [], "the second delivery must not re-ingest" + item = _doc(table) + assert item["status"] == "complete" + assert item["indexedAt"].startswith("2026-09-01T14:53:19") + + def test_a_small_document_still_finishes_in_one_invocation(self, table): + """The fast path must not regress into waiting for a retry. + + Deferring every document would add a minute of EventBridge backoff to the + common case, which is why the in-invocation wait exists at all. + """ + self._seed_managed(table) + fake = _FakeBackend(statuses=["NOT_FOUND", "INDEXED"]) + + with patch( + "apis.shared.kb_backend.managed_backend.ManagedKbBackend", return_value=fake + ): + result = ic.handle_object(BUCKET, KEY) + + assert result["ingested"] is True + assert _doc(table)["status"] == "complete" + assert fake.ingested == [DOCUMENT_ID] + + def test_a_failed_document_is_marked_failed_and_not_retried(self, table): + """Terminal on Bedrock's side. Redelivering cannot help.""" + self._seed_managed(table) + fake = _FakeBackend(statuses=["FAILED"]) + + with patch( + "apis.shared.kb_backend.managed_backend.ManagedKbBackend", return_value=fake + ): + result = ic.handle_object(BUCKET, KEY) # must NOT raise + + assert fake.ingested == [] + item = _doc(table) + assert item["status"] == "failed" + assert "FAILED" in item["ingestionError"] + assert result["status"] == "FAILED" + + def test_a_partially_indexed_document_is_treated_as_usable(self, table): + """It IS retrievable, so failing it would hide content the user can see.""" + self._seed_managed(table) + fake = _FakeBackend(statuses=["PARTIALLY_INDEXED"]) + + with patch( + "apis.shared.kb_backend.managed_backend.ManagedKbBackend", return_value=fake + ): + ic.handle_object(BUCKET, KEY) + + assert fake.ingested == [], "already indexed, even if partially" + assert _doc(table)["status"] == "complete" + + def test_a_status_probe_failure_does_not_fail_the_document(self, table): + """An unreadable probe means "no evidence", so ingesting is correct.""" + self._seed_managed(table) + + class _ProbeBroken(_FakeBackend): + def _agent(self): + raise RuntimeError("bedrock control plane unavailable") + + fake = _ProbeBroken(statuses=["NOT_FOUND"]) + with patch( + "apis.shared.kb_backend.managed_backend.ManagedKbBackend", return_value=fake + ), patch.object(ic, "INDEXED_POLL_TIMEOUT_SECONDS", 0.01): + with pytest.raises(ic.IngestionRoutingError): + ic.handle_object(BUCKET, KEY) + + assert fake.ingested == [DOCUMENT_ID], "a probe failure must not block ingestion" + assert _doc(table)["status"] != "failed" diff --git a/backend/tests/lambdas/test_kb_migration_worker.py b/backend/tests/lambdas/test_kb_migration_worker.py index 879651472..aa3553169 100644 --- a/backend/tests/lambdas/test_kb_migration_worker.py +++ b/backend/tests/lambdas/test_kb_migration_worker.py @@ -625,22 +625,51 @@ def test_a_document_with_no_hash_still_contributes_a_changing_value(self): assert worker.manifest_entry(item) == "d9:2026-08-01T00:00:00Z" @pytest.mark.asyncio - async def test_verify_requires_a_canary_retrieval_to_return_something(self): - """Requirement 15.7. Bedrock reporting a document INDEXED precedes it being - retrievable by 0.75-1.03 s, and a knowledge base can hold documents while - returning nothing, so "we ingested everything" and "retrieval works" are - separate claims.""" + async def test_an_unqueryable_corpus_defers_instead_of_failing(self): + """Requirement 15.7, corrected by measurement. + + "Not queryable yet" is a verification that has not happened, not one that + failed. The docstring's 0.75-1.03 s was measured on a warm knowledge base; + a first ingest into a fresh one took ~45 s in dev, and treating that as + terminal marked a good migration `failed` and showed its owner a retry + button for a problem that resolves itself. + """ + backend = StubBackend(chunks=[]) + deferred = [] + + def _defer(_assistant, _kb, _generation, due_at): + deferred.append(due_at) + return len(deferred) + + with patch.dict("os.environ", BASE_ENV, clear=True), patch.object( + worker, "list_document_items", return_value=[_doc("d1")] + ), patch.object(r, "defer_verify", _defer): + result = await worker.run_verify( + ASSISTANT_ID, ASSISTANT_ID, _kb_record(r.VERIFY), backend + ) + + assert deferred, "did not defer; an early canary would fail the migration" + assert result.to_state == r.VERIFY, "left verify on a deferral" + assert "not queryable" in result.detail + + @pytest.mark.asyncio + async def test_deferring_forever_eventually_fails(self): + """Bounded, because "not queryable" past some point is not latency. + + Matched on the attempt count rather than "not queryable", so this cannot + be satisfied by the deferral path it is meant to sit past. + """ backend = StubBackend(chunks=[]) with patch.dict("os.environ", BASE_ENV, clear=True), patch.object( worker, "list_document_items", return_value=[_doc("d1")] + ), patch.object( + r, "defer_verify", lambda *a, **k: worker.MAX_VERIFY_ATTEMPTS + 1 ): - # Matched on "not queryable", not on "canary": both failure messages - # mention the canary, so the looser pattern passed even with the - # empty-result check removed — the *other* check raised and the test - # could not tell the difference. - with pytest.raises(worker.VerificationFailed, match="not queryable"): - await worker.run_verify(ASSISTANT_ID, ASSISTANT_ID, _kb_record(r.VERIFY), backend) + with pytest.raises(worker.VerificationFailed, match="attempts over"): + await worker.run_verify( + ASSISTANT_ID, ASSISTANT_ID, _kb_record(r.VERIFY), backend + ) @pytest.mark.asyncio async def test_verify_rejects_a_canary_that_returns_foreign_documents(self): diff --git a/backend/tests/property/test_pbt_cleanup_service.py b/backend/tests/property/test_pbt_cleanup_service.py index cca1f099b..92d429d81 100644 --- a/backend/tests/property/test_pbt_cleanup_service.py +++ b/backend/tests/property/test_pbt_cleanup_service.py @@ -104,6 +104,15 @@ def failing_s3_delete(**kwargs): side_effect=failing_fallback, ), patch("boto3.client", return_value=mock_s3_client), + # These cases are all about a LEGACY knowledge base — the state of + # every assistant that predates the managed-KB migration. Absence of + # a KB record is what "legacy" looks like, so the managed deletion + # phase resolves to a no-op. Stated explicitly rather than left to a + # real DynamoDB read. + patch( + "apis.shared.kb_backend.records.get_kb_record", + return_value=None, + ), patch( "apis.app_api.documents.services.cleanup_service.asyncio.sleep", side_effect=mock_sleep, @@ -208,6 +217,15 @@ async def failing_fallback(*args, **kwargs): side_effect=failing_fallback, ), patch("boto3.client", return_value=mock_s3_client), + # These cases are all about a LEGACY knowledge base — the state of + # every assistant that predates the managed-KB migration. Absence of + # a KB record is what "legacy" looks like, so the managed deletion + # phase resolves to a no-op. Stated explicitly rather than left to a + # real DynamoDB read. + patch( + "apis.shared.kb_backend.records.get_kb_record", + return_value=None, + ), patch( "apis.app_api.documents.services.cleanup_service.asyncio.sleep", new_callable=AsyncMock, @@ -286,6 +304,15 @@ async def succeeding_fallback(*args, **kwargs): side_effect=succeeding_fallback, ), patch("boto3.client", return_value=mock_s3_client), + # These cases are all about a LEGACY knowledge base — the state of + # every assistant that predates the managed-KB migration. Absence of + # a KB record is what "legacy" looks like, so the managed deletion + # phase resolves to a no-op. Stated explicitly rather than left to a + # real DynamoDB read. + patch( + "apis.shared.kb_backend.records.get_kb_record", + return_value=None, + ), patch( "apis.app_api.documents.services.document_service.hard_delete_document", mock_hard_delete, diff --git a/backend/tests/routes/test_cleanup_service.py b/backend/tests/routes/test_cleanup_service.py index b4c78fa8c..9b392f2c3 100644 --- a/backend/tests/routes/test_cleanup_service.py +++ b/backend/tests/routes/test_cleanup_service.py @@ -45,6 +45,15 @@ async def test_cleanup_returns_true_when_both_succeed(self): new_callable=AsyncMock, ), patch("boto3.client", return_value=mock_s3_client), + # These cases are all about a LEGACY knowledge base — the state of + # every assistant that predates the managed-KB migration. Absence of + # a KB record is what "legacy" looks like, so the managed deletion + # phase resolves to a no-op. Stated explicitly rather than left to a + # real DynamoDB read. + patch( + "apis.shared.kb_backend.records.get_kb_record", + return_value=None, + ), patch( "apis.app_api.documents.services.document_service.hard_delete_document", mock_hard_delete, @@ -83,6 +92,15 @@ async def test_cleanup_returns_false_when_vectors_fail(self): side_effect=Exception("vector fallback error"), ), patch("boto3.client", return_value=mock_s3_client), + # These cases are all about a LEGACY knowledge base — the state of + # every assistant that predates the managed-KB migration. Absence of + # a KB record is what "legacy" looks like, so the managed deletion + # phase resolves to a no-op. Stated explicitly rather than left to a + # real DynamoDB read. + patch( + "apis.shared.kb_backend.records.get_kb_record", + return_value=None, + ), patch( "apis.app_api.documents.services.cleanup_service.asyncio.sleep", new_callable=AsyncMock, @@ -123,6 +141,15 @@ async def test_cleanup_returns_false_when_s3_fails(self): new_callable=AsyncMock, ), patch("boto3.client", return_value=mock_s3_client), + # These cases are all about a LEGACY knowledge base — the state of + # every assistant that predates the managed-KB migration. Absence of + # a KB record is what "legacy" looks like, so the managed deletion + # phase resolves to a no-op. Stated explicitly rather than left to a + # real DynamoDB read. + patch( + "apis.shared.kb_backend.records.get_kb_record", + return_value=None, + ), patch( "apis.app_api.documents.services.cleanup_service.asyncio.sleep", new_callable=AsyncMock, @@ -165,6 +192,15 @@ async def test_cleanup_independent_phases(self): side_effect=Exception("vector fallback error"), ), patch("boto3.client", return_value=mock_s3_client), + # These cases are all about a LEGACY knowledge base — the state of + # every assistant that predates the managed-KB migration. Absence of + # a KB record is what "legacy" looks like, so the managed deletion + # phase resolves to a no-op. Stated explicitly rather than left to a + # real DynamoDB read. + patch( + "apis.shared.kb_backend.records.get_kb_record", + return_value=None, + ), patch( "apis.app_api.documents.services.cleanup_service.asyncio.sleep", new_callable=AsyncMock, @@ -214,6 +250,15 @@ async def fail_twice_then_succeed(*args, **kwargs): new_callable=AsyncMock, ), patch("boto3.client", return_value=mock_s3_client), + # These cases are all about a LEGACY knowledge base — the state of + # every assistant that predates the managed-KB migration. Absence of + # a KB record is what "legacy" looks like, so the managed deletion + # phase resolves to a no-op. Stated explicitly rather than left to a + # real DynamoDB read. + patch( + "apis.shared.kb_backend.records.get_kb_record", + return_value=None, + ), patch( "apis.app_api.documents.services.cleanup_service.asyncio.sleep", new_callable=AsyncMock, @@ -256,6 +301,15 @@ async def test_cleanup_calls_hard_delete_on_success(self): new_callable=AsyncMock, ), patch("boto3.client", return_value=mock_s3_client), + # These cases are all about a LEGACY knowledge base — the state of + # every assistant that predates the managed-KB migration. Absence of + # a KB record is what "legacy" looks like, so the managed deletion + # phase resolves to a no-op. Stated explicitly rather than left to a + # real DynamoDB read. + patch( + "apis.shared.kb_backend.records.get_kb_record", + return_value=None, + ), patch( "apis.app_api.documents.services.document_service.hard_delete_document", mock_hard_delete, @@ -294,6 +348,15 @@ async def test_cleanup_does_not_call_hard_delete_on_failure(self): side_effect=Exception("vector fallback error"), ), patch("boto3.client", return_value=mock_s3_client), + # These cases are all about a LEGACY knowledge base — the state of + # every assistant that predates the managed-KB migration. Absence of + # a KB record is what "legacy" looks like, so the managed deletion + # phase resolves to a no-op. Stated explicitly rather than left to a + # real DynamoDB read. + patch( + "apis.shared.kb_backend.records.get_kb_record", + return_value=None, + ), patch( "apis.app_api.documents.services.cleanup_service.asyncio.sleep", new_callable=AsyncMock, diff --git a/backend/tests/security/test_python_ast_policy.py b/backend/tests/security/test_python_ast_policy.py index 3a52a7f78..39ffa52c9 100644 --- a/backend/tests/security/test_python_ast_policy.py +++ b/backend/tests/security/test_python_ast_policy.py @@ -10,6 +10,9 @@ * Dunder attribute access (``__class__``, ``__bases__``, ...) is rejected so the standard "walk the type hierarchy" obfuscation cannot reach ``__builtins__``. +* Attribute chains that resolve through an allowlisted module which itself + imports a host module (``pd.io.common.os.popen``) are rejected — the + disclosed bypass of the import/name-only checks. * Realistic chart and dataframe code is accepted unchanged. """ @@ -112,6 +115,104 @@ def test_dunder_attribute_access_rejected() -> None: validate_diagram_code(src) +# --------------------------------------------------------------------------- +# Attribute-chain re-export (regression: allowlisted module re-exports ``os``) +# --------------------------------------------------------------------------- + + +def test_reported_pandas_io_common_os_popen_chain_rejected() -> None: + """The disclosed payload: ``pandas.io.common`` does ``import os``, so the + attribute chain re-exposes the full module with no import statement.""" + src = textwrap.dedent(""" + import matplotlib; matplotlib.use('Agg'); import matplotlib.pyplot as plt + import pandas as pd + util = pd.io.common + out = util.os.popen('id; pwd; echo VERIFY-MARK-7Q4').read() + plt.figure(); plt.plot([1,2,3]); plt.savefig('verif_chart.png') + raise ValueError('LINEAGE-OUTPUT>>> ' + out) + """) + with pytest.raises(PolicyError): + validate_diagram_code(src) + + +@pytest.mark.parametrize( + "src", + [ + # Direct chain, no intermediate binding. + "import pandas as pd\npd.io.common.os.popen('id')", + # Chain split across locals to defeat a single-expression matcher. + "import pandas as pd\na = pd.io\nb = a.common\nc = b.os\nc.popen('id')", + # Same shape through other allowlisted packages. + "import matplotlib as m\nm.cbook.os.getcwd()", + "import numpy as np\nnp.lib.npyio.os.getcwd()", + # Submodule imported directly, then traversed. + "import pandas.io.common as u\nu.os.popen('id')", + "from pandas.io import common\ncommon.os.popen('id')", + # Re-exported module bound by name at import time. + "from pandas.io.common import os\nos.popen('id')", + "from pandas.io.common import os as o\no.popen('id')", + # Other host modules through the same door. + "import pandas as pd\npd.io.common.sys.modules", + "import pandas as pd\npd.compat.platform.uname()", + # Builtins namespace and object-graph traversal. + "import pandas as pd\npd.io.common.builtins.eval('1')", + "import numpy as np\nnp.core.gc.get_referrers(np)", + ], +) +def test_attribute_chain_to_host_module_rejected(src: str) -> None: + with pytest.raises(PolicyError): + validate_diagram_code(src) + + +@pytest.mark.parametrize( + "src", + [ + # Pickle sinks execute constructor code during load, and the payload + # can arrive as a bytes literal through an allowlisted buffer — no + # forbidden import, no file the attacker had to write. + "import pandas as pd\nimport io\npd.read_pickle(io.BytesIO(b'x'))", + "import pandas as pd\npd.DataFrame().to_pickle('/tmp/x')", + "import pandas as pd\nimport io\npd.read_hdf(io.BytesIO(b'x'))", + "import numpy as np\nimport io\nnp.load(io.BytesIO(b'x'), allow_pickle=True)", + # Bound at import time instead of reached as an attribute. + "from pandas import read_pickle\nread_pickle('/tmp/x')", + "from numpy import load\nload('/tmp/x', allow_pickle=True)", + ], +) +def test_deserialization_sinks_rejected(src: str) -> None: + with pytest.raises(PolicyError): + validate_diagram_code(src) + + +def test_process_spawn_attribute_rejected_even_on_unknown_object() -> None: + """Second net: if a host module is reachable under a name the policy did + not enumerate, the spawn call itself is still refused.""" + with pytest.raises(PolicyError): + validate_diagram_code("import pandas as pd\npd.io.parsers.mod.popen('id')") + + +def test_column_named_like_a_denied_name_uses_subscript() -> None: + """Documented consequence of the attribute rule: attribute access to a + column that collides with a denied name is refused; the subscript form + (which the model can always emit) is accepted.""" + denied = textwrap.dedent(""" + import pandas as pd + df = pd.read_csv('ohlc.csv') + df.open.plot() + """) + with pytest.raises(PolicyError): + validate_diagram_code(denied) + + allowed = textwrap.dedent(""" + import pandas as pd + import matplotlib.pyplot as plt + df = pd.read_csv('ohlc.csv') + df['open'].plot() + plt.savefig('ohlc.png') + """) + validate_diagram_code(allowed) + + def test_dunder_access_on_user_object_rejected() -> None: src = textwrap.dedent(""" class A: @@ -249,3 +350,23 @@ def test_multiline_with_comments_and_lambdas_passes() -> None: plt.savefig('q.png') """) validate_diagram_code(src) + + +def test_scipy_submodule_via_direct_import_passes() -> None: + """A lazily-loaded submodule whose name collides with a denied module + (``scipy.signal`` vs. the stdlib ``signal``) is reachable by importing it + directly — the attribute form is not, in either direction.""" + src = textwrap.dedent(""" + import matplotlib.pyplot as plt + import numpy as np + from scipy.signal import butter, filtfilt + + b, a = butter(3, 0.2) + y = filtfilt(b, a, np.linspace(0, 1, 200)) + plt.plot(y) + plt.savefig('filtered.png') + """) + validate_diagram_code(src) + + with pytest.raises(PolicyError): + validate_diagram_code("import scipy as sp\nsp.signal.butter(3, 0.2)") diff --git a/backend/tests/shared/test_managed_kb_backend.py b/backend/tests/shared/test_managed_kb_backend.py index db6ac3411..f82ec3be9 100644 --- a/backend/tests/shared/test_managed_kb_backend.py +++ b/backend/tests/shared/test_managed_kb_backend.py @@ -57,6 +57,16 @@ # --------------------------------------------------------------------------- +def _conflict(message: str) -> Exception: + """A ClientError shaped like the real ConflictException.""" + from botocore.exceptions import ClientError + + return ClientError( + {"Error": {"Code": "ConflictException", "Message": message}}, + "CreateKnowledgeBase", + ) + + class FakeBedrockAgent: """A ``bedrock-agent`` control-plane stub with real idempotency semantics. @@ -74,9 +84,17 @@ def __init__( *, on_create=None, create_failures: Optional[List[Exception]] = None, + status_sequence: Optional[List[str]] = None, + token_still_valid: bool = False, ) -> None: self.create_kb_calls: List[Dict[str, Any]] = [] self.create_ds_calls: List[Dict[str, Any]] = [] + self.get_kb_calls: List[Dict[str, Any]] = [] + self.list_kb_calls: List[Dict[str, Any]] = [] + self._by_name: Dict[str, str] = {} + self._names_by_id: Dict[str, str] = {} + self.token_still_valid = token_still_valid + self._status_sequence = list(status_sequence or ["ACTIVE"]) self.ingest_calls: List[Dict[str, Any]] = [] self.delete_calls: List[Dict[str, Any]] = [] self.start_ingestion_job_calls: List[Dict[str, Any]] = [] @@ -102,10 +120,63 @@ def create_knowledge_base(self, **kwargs): raise self._create_failures.pop(0) token = kwargs["clientToken"] - if token not in self._by_token: - self._counter += 1 - self._by_token[token] = f"KB{self._counter:08d}" - return {"knowledgeBase": {"knowledgeBaseId": self._by_token[token], "status": "ACTIVE"}} + name = kwargs["name"] + + # NAME UNIQUENESS, which is what AWS actually enforces and what this fake + # used to ignore. `clientToken` deduplication is real but *expires* within + # minutes, so a retry an hour later is a new request that collides on the + # name. Modelling the token as permanent is why two tests certified a + # design that could not recover: the first real migration created a + # knowledge base, failed before recording its id, and every retry + # thereafter was refused with "already exists". + # + # `token_still_valid` selects which side of that expiry is being modelled. + # It defaults to False — the realistic case for any retry that is not + # within the same few minutes. + if token in self._by_token and self.token_still_valid: + return { + "knowledgeBase": { + "knowledgeBaseId": self._by_token[token], + "status": "CREATING", + } + } + if name in self._by_name: + raise _conflict(f"KnowledgeBase with name {name} already exists.") + + self._counter += 1 + kb_id = f"KB{self._counter:08d}" + self._by_token[token] = kb_id + self._by_name[name] = kb_id + self._names_by_id[kb_id] = name + # CREATING, not ACTIVE — what the real API returns. The fake previously + # claimed ACTIVE here, which is why nothing caught the provisioner calling + # CreateDataSource against a knowledge base that was still creating. + return {"knowledgeBase": {"knowledgeBaseId": kb_id, "status": "CREATING"}} + + def list_knowledge_bases(self, **kwargs): + """Summaries, so adopt-by-name can find a knowledge base it did not record.""" + self.list_kb_calls.append(kwargs) + return { + "knowledgeBaseSummaries": [ + {"knowledgeBaseId": kb_id, "name": name, "status": "ACTIVE"} + for kb_id, name in self._names_by_id.items() + ] + } + + def get_knowledge_base(self, **kwargs): + """Status poll. Yields each queued status once, then settles on the last. + + Default is a single ACTIVE so tests that do not care about the wait are + unaffected; `status_sequence` lets one drive CREATING -> ACTIVE or a + terminal failure. + """ + self.thread_idents.append(threading.get_ident()) + self.get_kb_calls.append(kwargs) + if len(self._status_sequence) > 1: + status = self._status_sequence.pop(0) + else: + status = self._status_sequence[0] + return {"knowledgeBase": {"knowledgeBaseId": kwargs["knowledgeBaseId"], "status": status}} def create_data_source(self, **kwargs): self.thread_idents.append(threading.get_ident()) @@ -363,22 +434,42 @@ def test_storage_configuration_is_omitted_entirely(self): def test_role_arn_is_passed(self): assert self._payload()["roleArn"] == ROLE_ARN - def test_embedding_is_pinned_to_titan_v2_float32_1024(self): - """Requirement 8.5, and immutable from here on (8.8). + def test_no_embedding_pin_is_sent(self): + """Requirement 8.5, as amended. + + The pin was carried over from the legacy path without re-deriving it, and + it does not apply here: + + * On S3 Vectors *we* embed the user's question, so the query model must + match the model that indexed the documents. Managed retrieval sends + text and managed ingestion sends text — we never produce a vector, so + Bedrock embeds both sides and consistency is its invariant. + * AWS rejects the pin together with ``rerankingModelType: MANAGED``, and + the evaluation measured the pin as worth nothing ("identical answer + quality — 9/9") against reranking being "what makes a small context cap + defensible". - A drift in any of these three values is not a migration but a rebuild, so - the numbers are asserted rather than merely present. + Asserted as *absence*, because sending any of these keys is what breaks + reranking — and the failure is a ValidationException at query time, long + after the immutable choice was made. """ managed = self._payload()["knowledgeBaseConfiguration"][ "managedKnowledgeBaseConfiguration" ] - assert managed["embeddingModelType"] == "CUSTOM" - assert managed["embeddingModelArn"].endswith("amazon.titan-embed-text-v2:0") - bedrock_config = managed["embeddingModelConfiguration"][ - "bedrockEmbeddingModelConfiguration" - ] - assert bedrock_config["dimensions"] == 1024 - assert bedrock_config["embeddingDataType"] == "FLOAT32" + assert "embeddingModelType" not in managed + assert "embeddingModelArn" not in managed + assert "embeddingModelConfiguration" not in managed + + def test_reranking_stays_managed(self): + """The half of the tradeoff that has evidence behind it (Req 11.2). + + Kept next to the pin test on purpose: these two are mutually exclusive in + AWS, so anyone reinstating the pin should see this failing beside it. + """ + from apis.shared.kb_backend.managed_backend import retrieval_configuration + + managed = retrieval_configuration()["managedSearchConfiguration"] + assert managed["rerankingModelType"] == "MANAGED" def test_kms_key_is_only_sent_when_supplied(self): assert "serverSideEncryptionConfiguration" not in self._payload()[ @@ -591,7 +682,13 @@ def _lose(assistant_id, record): ) @pytest.mark.asyncio - async def test_provisioning_requires_a_service_role(self, table): + async def test_provisioning_requires_a_service_role(self, table, monkeypatch): + # `role_arn=None` falls back to MANAGED_KB_SERVICE_ROLE_ARN, so this only + # asserted what it meant while that variable happened to be absent from the + # environment. It is now present in `backend/src/.env` for the local + # migration driver — which `load_dotenv(override=True)` reads — so the + # absence has to be made explicit rather than assumed. + monkeypatch.delenv("MANAGED_KB_SERVICE_ROLE_ARN", raising=False) with pytest.raises(p.ProvisioningError, match="service role"): await _provision(FakeBedrockAgent(), role_arn=None) @@ -663,10 +760,24 @@ class TestCrashBetweenCreateAndRecordUpdate: The window that record-first ordering exists to make survivable: the AWS knowledge base exists, the record does not yet name it. + + ⚠️ These tests previously certified the opposite of what they now assert, and + that is how the first real migration became permanently unretryable. They + asserted `"awsKbId" not in anchor` — the missing write, enshrined as a + requirement — and leaned on `clientToken` deduplication to make the retry + safe, which the fake modelled as **permanent**. Real AWS idempotency tokens + expire within minutes, so the retry was a new request that collided on the + unique name and was refused forever: + + ConflictException: KnowledgeBase with name ... already exists. + + The identifier is now persisted the moment it exists, which is what actually + makes the window survivable. The token is a nice-to-have inside the expiry + window; it is not the mechanism. """ @pytest.mark.asyncio - async def test_the_record_survives_as_a_discoverable_retry_anchor(self, table): + async def test_the_identifier_is_recorded_before_anything_else_can_fail(self, table): client = FakeBedrockAgent() crashed = [] @@ -691,15 +802,15 @@ def _crash(*_args, **_kwargs): "orphan nothing can find (Requirement 7.8)" ) assert anchor["provisioningState"] == r.PROVISIONING - assert "awsKbId" not in anchor - assert anchor["clientToken"], ( - "the anchor carries no clientToken, so a retry cannot be deduplicated " - "and would create a second knowledge base" + assert anchor.get("awsKbId") == "KB00000001", ( + "the identifier was not recorded, so a later retry cannot resume from " + "it and will be refused because the name is already taken" ) @pytest.mark.asyncio - async def test_the_retry_does_not_create_a_second_knowledge_base(self, table): - """The whole point: one knowledge base across a crash and a retry.""" + async def test_the_retry_resumes_instead_of_re_creating(self, table): + """One knowledge base across a crash and a retry — by resuming, not by + re-issuing a create and hoping AWS deduplicates it.""" client = FakeBedrockAgent() original = r.attach_aws_ids @@ -712,12 +823,69 @@ async def test_the_retry_does_not_create_a_second_knowledge_base(self, table): result = await _provision(client) # the retry - assert len(client.create_kb_calls) == 2, "the retry did not re-issue the create" - assert client.distinct_knowledge_base_ids == {"KB00000001"}, ( - "the retry created a SECOND knowledge base: the persisted clientToken " - "was not reused, so AWS did not deduplicate" + assert len(client.create_kb_calls) == 1, ( + "the retry re-issued CreateKnowledgeBase. With an expired idempotency " + "token that is refused outright — the name is taken — so resuming from " + "the recorded id is the only thing that works" ) + assert client.distinct_knowledge_base_ids == {"KB00000001"} assert result.aws_kb_id == "KB00000001" + + @pytest.mark.asyncio + async def test_adoption_ignores_a_knowledge_base_being_deleted(self, table): + """Adopting a dying knowledge base guarantees a failure one step later. + + Seen while recreating one locally: the delete had not finished, adoption + took the DELETING knowledge base, and the ACTIVE wait then refused it. The + name is about to free up, so skipping is correct — the next attempt creates + fresh. + """ + from apis.shared.kb_backend import provisioning as prov + + class _Dying: + def list_knowledge_bases(self, **_kwargs): + return { + "knowledgeBaseSummaries": [ + {"knowledgeBaseId": "KBDYING", "name": "wanted", + "status": "DELETING"}, + ] + } + + found = await prov._find_knowledge_base_by_name(_Dying(), "wanted") + assert found is None, "adopted a knowledge base that is being deleted" + + @pytest.mark.asyncio + async def test_a_lost_identifier_is_recovered_by_adopting_the_name(self, table): + """The state the first real migration was actually stuck in. + + The knowledge base exists in AWS, the record has no `awsKbId` (it was + written before this fix), and the token has long expired. Without + adopt-by-name the create is refused forever and the migration can never + succeed — the operator's only recourse would be deleting the knowledge + base by hand. + """ + client = FakeBedrockAgent() + await _provision(client) # creates KB00000001 and records it + + # Simulate the pre-fix record: identifier dropped, still provisioning. + table.update_item( + Key={"PK": r.kb_pk(ASSISTANT_ID), "SK": r.kb_sk(APP_KB_ID)}, + UpdateExpression="REMOVE awsKbId, awsDataSourceId SET provisioningState = :p", + ExpressionAttributeValues={":p": r.PROVISIONING}, + ) + + result = await _provision(client) + + assert result.aws_kb_id == "KB00000001", "did not adopt the existing name" + assert client.distinct_knowledge_base_ids == {"KB00000001"}, ( + "a second knowledge base was created; the name collision should have " + "been resolved by adoption, not by another create" + ) + assert client.list_kb_calls, "adoption did not look the name up" + assert _record(table).get("awsKbId") == "KB00000001", ( + "the adopted identifier was not persisted, so the next attempt would " + "have to adopt all over again" + ) assert _record(table)["provisioningState"] == r.ACTIVE @pytest.mark.asyncio @@ -747,6 +915,89 @@ async def test_a_crash_after_the_data_source_still_converges(self, table): # =========================================================================== +class TestWaitsForActiveBeforeTheDataSource: + """The defect that orphaned the first real knowledge base in dev. + + `CreateKnowledgeBase` returns while the knowledge base is `CREATING` — this + module's header records 47-124 s to `ACTIVE` (n=7) — and `CreateDataSource` + against a creating knowledge base is refused: + + ConflictException: The Knowledge Base is not in a valid status. + + The create succeeded, the data source did not, `attach_aws_ids` never ran, and + the knowledge base was left in AWS with nothing pointing at it. + + Nothing caught it because the fake returned `ACTIVE` from `create_knowledge_base`, + which the real API never does. The fake now returns `CREATING`, so these tests + exercise the wait rather than skipping past it. + """ + + @pytest.mark.asyncio + async def test_the_data_source_is_created_only_after_active(self, table): + client = FakeBedrockAgent(status_sequence=["CREATING", "CREATING", "ACTIVE"]) + await _provision(client) + + assert client.create_kb_calls, "no knowledge base was created" + assert client.create_ds_calls, "no data source was created" + # Polled until ACTIVE rather than charging ahead. + assert len(client.get_kb_calls) == 3, ( + f"expected three status polls, saw {len(client.get_kb_calls)}" + ) + + @pytest.mark.asyncio + async def test_no_data_source_while_the_knowledge_base_is_creating(self, table): + """The precondition is waited on, not retried through.""" + client = FakeBedrockAgent(status_sequence=["CREATING"]) + with pytest.raises(p.KnowledgeBaseNotReady, match="still CREATING"): + await _provision(client, budget_seconds=10.0, interval_seconds=5.0) + + assert client.create_kb_calls, "the knowledge base should still be created" + assert client.create_ds_calls == [], ( + "CreateDataSource must not be attempted against a CREATING knowledge " + "base — that is the ConflictException this wait exists to prevent" + ) + + @pytest.mark.asyncio + async def test_a_terminal_status_fails_immediately(self, table): + """FAILED will never become ACTIVE, so waiting only delays the report.""" + client = FakeBedrockAgent(status_sequence=["FAILED"]) + with pytest.raises(p.KnowledgeBaseNotReady, match="FAILED"): + await _provision(client, budget_seconds=300.0, interval_seconds=5.0) + + assert len(client.get_kb_calls) == 1, ( + "a terminal status should be acted on after one poll, not waited out" + ) + assert client.create_ds_calls == [] + + @pytest.mark.asyncio + async def test_the_budget_is_read_at_call_time(self, table): + """Bound as a default argument the budget would be unpatchable. + + Asserted by giving two calls different budgets on the same import. + """ + slow = FakeBedrockAgent(status_sequence=["CREATING"]) + with pytest.raises(p.KnowledgeBaseNotReady): + await _provision(slow, budget_seconds=5.0, interval_seconds=5.0) + few = len(slow.get_kb_calls) + + slower = FakeBedrockAgent(status_sequence=["CREATING"]) + with pytest.raises(p.KnowledgeBaseNotReady): + await _provision(slower, budget_seconds=25.0, interval_seconds=5.0) + + assert len(slower.get_kb_calls) > few, ( + "a larger budget polled no more than a smaller one, so the value is " + "not being read at call time" + ) + + @pytest.mark.asyncio + async def test_the_failure_message_says_the_retry_is_safe(self, table): + """An operator reading this needs to know a retry will not duplicate.""" + client = FakeBedrockAgent(status_sequence=["CREATING"]) + with pytest.raises(p.KnowledgeBaseNotReady) as excinfo: + await _provision(client, budget_seconds=5.0, interval_seconds=5.0) + assert "already recorded on the KB_Record" in str(excinfo.value) + + class TestOffEventLoop: @pytest.mark.asyncio async def test_create_knowledge_base_runs_in_a_worker_thread(self, table): diff --git a/backend/tests/shared/test_session_cross_user_fork.py b/backend/tests/shared/test_session_cross_user_fork.py new file mode 100644 index 000000000..7196bc3b3 --- /dev/null +++ b/backend/tests/shared/test_session_cross_user_fork.py @@ -0,0 +1,139 @@ +"""A session id must never be forked across two users. + +Prod, 2026-08-31: someone opened the CIO's `/s/{sessionId}` link. The metadata +read 404'd, the SPA treated the session as new, and the turn that followed +created a SECOND metadata row on the same session id under the second user — +`ensure_session_metadata_exists`'s `attribute_not_exists(PK)` guard cannot see +it, because the new row has a different PK. + +Not a confidentiality bug: conversation content is keyed by actor id in +AgentCore Memory, so the second user only ever saw an empty thread. The damage +was the duplicate row, the billing attached to it, and the original owner's +session resolving non-deterministically between the two rows afterwards. +""" + +import pytest + +from apis.shared.sessions.models import SessionMetadata + + +def _meta(session_id="s1", user_id="owner", **kw): + defaults = dict( + sessionId=session_id, userId=user_id, title="Test Session", + status="active", createdAt="2026-01-01T00:00:00Z", + lastMessageAt="2026-01-01T00:00:00Z", messageCount=1, + ) + defaults.update(kw) + return SessionMetadata(**defaults) + + +def _put_forked_row(table, session_id: str, user_id: str, title: str) -> None: + """Write a second META row for a session id, bypassing the write path. + + Reproduces the rows the platform created before `session_owned_by_other_user` + existed. Both rows carry identical GSI_PK/GSI_SK, so DynamoDB returns them + in an unspecified order — which is exactly the condition the item-scan in + `_get_session_by_gsi` has to survive. + """ + table.put_item( + Item={ + "PK": f"USER#{user_id}", + "SK": f"S#{session_id}", + "GSI_PK": f"SESSION#{session_id}", + "GSI_SK": "META", + "sessionId": session_id, + "userId": user_id, + "title": title, + "status": "active", + "createdAt": "2026-01-01T00:00:00Z", + "lastMessageAt": "2026-01-01T00:00:00Z", + "messageCount": 0, + "starred": False, + "tags": [], + } + ) + + +class TestSessionOwnedByOtherUser: + @pytest.mark.asyncio + async def test_false_when_no_session_exists(self, sessions_metadata_table): + from apis.shared.sessions.metadata import session_owned_by_other_user + assert await session_owned_by_other_user("nope", "someone") is False + + @pytest.mark.asyncio + async def test_false_for_the_owner(self, sessions_metadata_table): + from apis.shared.sessions.metadata import ( + store_session_metadata, + session_owned_by_other_user, + ) + await store_session_metadata(session_id="s1", user_id="owner", session_metadata=_meta()) + assert await session_owned_by_other_user("s1", "owner") is False + + @pytest.mark.asyncio + async def test_true_for_a_stranger(self, sessions_metadata_table): + from apis.shared.sessions.metadata import ( + store_session_metadata, + session_owned_by_other_user, + ) + await store_session_metadata(session_id="s1", user_id="owner", session_metadata=_meta()) + assert await session_owned_by_other_user("s1", "stranger") is True + + +class TestEnsureRefusesToFork: + @pytest.mark.asyncio + async def test_stranger_does_not_get_a_second_metadata_row(self, sessions_metadata_table): + from apis.shared.sessions.metadata import ( + store_session_metadata, + ensure_session_metadata_exists, + ) + await store_session_metadata(session_id="s1", user_id="owner", session_metadata=_meta()) + + created = await ensure_session_metadata_exists("s1", "stranger") + + assert created is False + metas = [ + i for i in sessions_metadata_table.scan()["Items"] + if i.get("GSI_SK") == "META" and i.get("sessionId") == "s1" + ] + assert len(metas) == 1 + assert metas[0]["userId"] == "owner" + + @pytest.mark.asyncio + async def test_owner_is_unaffected(self, sessions_metadata_table): + """The guard must not block the legitimate first-turn create.""" + from apis.shared.sessions.metadata import ensure_session_metadata_exists + assert await ensure_session_metadata_exists("fresh", "owner") is True + + +class TestLookupIsDeterministicAcrossAnExistingFork: + """Forked rows already exist in prod, so the read path has to cope.""" + + @pytest.mark.asyncio + async def test_each_user_resolves_to_their_own_row(self, sessions_metadata_table): + from apis.shared.sessions.metadata import store_session_metadata, get_session_metadata + + await store_session_metadata( + session_id="s1", user_id="owner", + session_metadata=_meta(title="Owner's conversation"), + ) + # The stranger's row is written RAW. The write path now refuses to + # create it, so this is the only way to reproduce what is already + # sitting in prod from before the guard existed. + _put_forked_row(sessions_metadata_table, "s1", "stranger", "New Conversation") + + owner_view = await get_session_metadata("s1", "owner") + stranger_view = await get_session_metadata("s1", "stranger") + + # Before the item-scan fix this depended on which row items[0] returned, + # and the owner could lose their own session. + assert owner_view is not None + assert owner_view.title == "Owner's conversation" + assert stranger_view is not None + assert stranger_view.title == "New Conversation" + + @pytest.mark.asyncio + async def test_third_party_still_sees_nothing(self, sessions_metadata_table): + from apis.shared.sessions.metadata import store_session_metadata, get_session_metadata + await store_session_metadata(session_id="s1", user_id="owner", session_metadata=_meta()) + _put_forked_row(sessions_metadata_table, "s1", "stranger", "New Conversation") + assert await get_session_metadata("s1", "outsider") is None diff --git a/backend/tests/shared/test_sessions_metadata.py b/backend/tests/shared/test_sessions_metadata.py index 96533ac3d..2b46d8694 100644 --- a/backend/tests/shared/test_sessions_metadata.py +++ b/backend/tests/shared/test_sessions_metadata.py @@ -1359,3 +1359,124 @@ def Table(self, _name): sessions, _ = await md.list_user_sessions("u1") assert [s.session_id for s in sessions] == ["leg"] # fell back to legacy, not blank + + +class TestPendingAttachmentsMarker: + """Write-ahead marker that lets a failed turn's attachments be re-sent. + + Regression cover for prod session `5f34d2b0` (2026-08-31): a ConverseStream + carrying two PDFs failed with ServiceUnavailableException, the inline bytes + were stripped from restored history, and the user had to re-upload by hand. + """ + + @pytest.mark.asyncio + async def test_set_then_pop_returns_ids_and_clears(self, sessions_metadata_table): + from apis.shared.sessions.metadata import ( + store_session_metadata, + set_pending_attachments, + pop_pending_attachments, + ) + await store_session_metadata(session_id="s1", user_id="u1", session_metadata=_make_session_metadata()) + + await set_pending_attachments("s1", "u1", ["up-1", "up-2"]) + assert await pop_pending_attachments("s1", "u1") == ["up-1", "up-2"] + + # The pop cleared it — a second turn must not recover them again. + assert await pop_pending_attachments("s1", "u1") == [] + + @pytest.mark.asyncio + async def test_pop_with_no_marker_returns_empty(self, sessions_metadata_table): + from apis.shared.sessions.metadata import store_session_metadata, pop_pending_attachments + await store_session_metadata(session_id="s1", user_id="u1", session_metadata=_make_session_metadata()) + assert await pop_pending_attachments("s1", "u1") == [] + + @pytest.mark.asyncio + async def test_clear_prevents_recovery_after_a_successful_turn(self, sessions_metadata_table): + """The success path clears the marker, so the next turn re-sends nothing.""" + from apis.shared.sessions.metadata import ( + store_session_metadata, + set_pending_attachments, + clear_pending_attachments, + pop_pending_attachments, + ) + await store_session_metadata(session_id="s1", user_id="u1", session_metadata=_make_session_metadata()) + + await set_pending_attachments("s1", "u1", ["up-1"]) + await clear_pending_attachments("s1", "u1") + assert await pop_pending_attachments("s1", "u1") == [] + + @pytest.mark.asyncio + async def test_stale_marker_is_discarded_but_still_cleared(self, sessions_metadata_table): + """A session abandoned for hours must not silently re-send documents + onto an unrelated follow-up question.""" + from datetime import datetime, timedelta, timezone + + from apis.shared.sessions.metadata import ( + PENDING_ATTACHMENT_RECOVERY_TTL_SECONDS, + store_session_metadata, + set_pending_attachments, + pop_pending_attachments, + ) + await store_session_metadata(session_id="s1", user_id="u1", session_metadata=_make_session_metadata()) + await set_pending_attachments("s1", "u1", ["up-1"]) + + stale = datetime.now(timezone.utc) - timedelta( + seconds=PENDING_ATTACHMENT_RECOVERY_TTL_SECONDS + 60 + ) + sessions_metadata_table.update_item( + Key={"PK": "USER#u1", "SK": "S#s1"}, + UpdateExpression="SET pendingAttachmentsAt = :t", + ExpressionAttributeValues={":t": stale.isoformat()}, + ) + + assert await pop_pending_attachments("s1", "u1") == [] + # Still cleared, so it can't linger and fire later. + assert "pendingAttachmentUploadIds" not in sessions_metadata_table.get_item( + Key={"PK": "USER#u1", "SK": "S#s1"} + )["Item"] + + @pytest.mark.asyncio + async def test_empty_upload_ids_writes_nothing(self, sessions_metadata_table): + from apis.shared.sessions.metadata import ( + store_session_metadata, + set_pending_attachments, + pop_pending_attachments, + ) + await store_session_metadata(session_id="s1", user_id="u1", session_metadata=_make_session_metadata()) + await set_pending_attachments("s1", "u1", []) + assert await pop_pending_attachments("s1", "u1") == [] + + @pytest.mark.asyncio + async def test_missing_session_is_a_noop(self, sessions_metadata_table): + """Best-effort contract: no session row → no write, no raise.""" + from apis.shared.sessions.metadata import set_pending_attachments, pop_pending_attachments + await set_pending_attachments("nope", "u1", ["up-1"]) + assert await pop_pending_attachments("nope", "u1") == [] + + @pytest.mark.asyncio + async def test_marker_is_scoped_to_its_own_session(self, sessions_metadata_table): + from apis.shared.sessions.metadata import ( + store_session_metadata, + set_pending_attachments, + pop_pending_attachments, + ) + await store_session_metadata(session_id="s1", user_id="u1", session_metadata=_make_session_metadata()) + await store_session_metadata(session_id="s2", user_id="u1", session_metadata=_make_session_metadata(session_id="s2")) + + await set_pending_attachments("s1", "u1", ["up-1"]) + assert await pop_pending_attachments("s2", "u1") == [] + assert await pop_pending_attachments("s1", "u1") == ["up-1"] + + @pytest.mark.asyncio + async def test_marker_survives_the_metadata_read_model(self, sessions_metadata_table): + from apis.shared.sessions.metadata import ( + store_session_metadata, + get_session_metadata, + set_pending_attachments, + ) + await store_session_metadata(session_id="s1", user_id="u1", session_metadata=_make_session_metadata()) + await set_pending_attachments("s1", "u1", ["up-1", "up-2"]) + + meta = await get_session_metadata("s1", "u1") + assert meta.pending_attachment_upload_ids == ["up-1", "up-2"] + assert meta.pending_attachments_at is not None diff --git a/backend/tests/supply_chain/test_kb_migration_env_contract.py b/backend/tests/supply_chain/test_kb_migration_env_contract.py index 9b48bb9ee..e535343bc 100644 --- a/backend/tests/supply_chain/test_kb_migration_env_contract.py +++ b/backend/tests/supply_chain/test_kb_migration_env_contract.py @@ -201,3 +201,64 @@ def test_no_kb_migration_variable_is_published_unread(self): def test_known_load_bearing_variables_stay_wired(self, name): """Spot-pins for variables whose absence is silent rather than loud.""" assert name in _names_set_by_construct() + + +# --------------------------------------------------------------------------- +# The poll budget must fit inside the Lambda that runs it +# --------------------------------------------------------------------------- +class TestTheIngestionPollBudgetFitsTheLambdaTimeout: + """A wait longer than the timeout is a killed invocation, not a wait. + + The consumer waits for Bedrock to finish indexing inside a single invocation, + because Lambda's asynchronous retry is capped at 2 attempts and cannot be + extended — so redelivery spans only minutes and a slow document would + dead-letter. That makes the in-invocation budget load-bearing, and it now lives + in two files that have no compiler between them: the timeout in the CDK + construct and the poll constants in Python. + + Raise either past the other and a slow-but-succeeding document is killed + mid-wait and dead-lettered — which is exactly the failure this budget was + introduced to remove. Hence a test rather than a comment. + """ + + def _lambda_timeout_minutes(self) -> int: + import re + + text = CONSTRUCT.read_text(encoding="utf-8") + # The consumer's own timeout, not another function's: anchor on its + # construct id and read the first timeout that follows. + start = text.index("KbIngestionConsumerLambda'") + match = re.search(r"timeout:\s*cdk\.Duration\.minutes\((\d+)\)", text[start:]) + assert match, "could not find the ingestion consumer's timeout in the construct" + return int(match.group(1)) + + def test_the_poll_budget_leaves_headroom_under_the_lambda_timeout(self): + from apis.app_api.kb_migration import ingestion_consumer as ic + + budget = ic.INDEXED_POLL_TIMEOUT_SECONDS + ic.RETRIEVABLE_POLL_TIMEOUT_SECONDS + timeout = self._lambda_timeout_minutes() * 60 + + assert budget < timeout, ( + f"the consumer can wait {budget:.0f}s but its Lambda times out at " + f"{timeout}s — a slow document would be killed mid-wait and " + f"dead-lettered, which is the bug this budget exists to prevent" + ) + # Headroom for the ingest call, the S3 read and cold start. + assert timeout - budget >= 120, ( + f"only {timeout - budget:.0f}s of headroom between the poll budget and " + f"the Lambda timeout; leave at least 120s for the ingest call itself" + ) + + def test_the_budget_covers_the_measured_indexing_tail(self): + """264 s was the slowest PDF in the §5.1 benchmark; dev saw 5 m 30 s. + + Pinned as a literal rather than compared to a constant, because asserting a + constant against itself proves nothing. This number is a property of + Bedrock's indexing behaviour, not a knob. + """ + from apis.app_api.kb_migration import ingestion_consumer as ic + + assert ic.INDEXED_POLL_TIMEOUT_SECONDS >= 330, ( + "the budget no longer covers the 5 m 30 s indexing time observed in dev " + "for a 1.5 MB PDF" + ) diff --git a/backend/uv.lock b/backend/uv.lock index 761c68e50..71d220092 100644 --- a/backend/uv.lock +++ b/backend/uv.lock @@ -12,7 +12,7 @@ resolution-markers = [ [[package]] name = "agentcore-stack" -version = "1.16.0" +version = "1.17.0" source = { editable = "." } dependencies = [ { name = "aiofiles" }, diff --git a/frontend/ai.client/package-lock.json b/frontend/ai.client/package-lock.json index 81a38e915..300f0c354 100644 --- a/frontend/ai.client/package-lock.json +++ b/frontend/ai.client/package-lock.json @@ -1,12 +1,12 @@ { "name": "ai.client", - "version": "1.16.0", + "version": "1.17.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "ai.client", - "version": "1.16.0", + "version": "1.17.0", "dependencies": { "@angular/cdk": "21.2.14", "@angular/common": "21.2.17", diff --git a/frontend/ai.client/package.json b/frontend/ai.client/package.json index fd819be54..93f9a805b 100644 --- a/frontend/ai.client/package.json +++ b/frontend/ai.client/package.json @@ -1,6 +1,6 @@ { "name": "ai.client", - "version": "1.16.0", + "version": "1.17.0", "scripts": { "ng": "ng", "start": "ng serve", diff --git a/frontend/ai.client/src/app/admin/skills/pages/skill-form.page.ts b/frontend/ai.client/src/app/admin/skills/pages/skill-form.page.ts index e1a19930b..60874c657 100644 --- a/frontend/ai.client/src/app/admin/skills/pages/skill-form.page.ts +++ b/frontend/ai.client/src/app/admin/skills/pages/skill-form.page.ts @@ -30,6 +30,11 @@ import { parseSkillMarkdown, slugifySkillId, } from '../models/skill-import.util'; +import { + DISALLOWED_RESOURCE_MESSAGE, + RESOURCE_ACCEPT_ATTR, + isAllowedResourceFilename, +} from '../../../shared/skills/skill-resource-types'; @Component({ selector: 'app-skill-form', @@ -233,7 +238,7 @@ import { id="refUpload" type="file" multiple - accept=".md,.markdown,.txt,text/markdown,text/plain" + [attr.accept]="acceptedFileTypes" class="sr-only" [disabled]="resourceBusy()" (change)="onRefFilesSelected($event)" @@ -407,6 +412,8 @@ export class SkillFormPage implements OnInit { readonly resources = signal([]); readonly pendingFiles = signal([]); readonly resourceBusy = signal(false); + /** `accept` filter for the file picker — mirrors the server allowlist. */ + readonly acceptedFileTypes = RESOURCE_ACCEPT_ATTR; readonly viewing = signal<{ filename: string; content: string } | null>(null); // Inline new-file authoring. @@ -530,6 +537,16 @@ export class SkillFormPage implements OnInit { * after the skill is created, since uploads need a skill_id). */ private async acceptFiles(files: File[]): Promise { + // Mirror of the backend type allowlist (see `resource_types.py`). The + // server rejects these with a 400 regardless; refusing here gives the + // admin a specific message instead of a failed round-trip. + const disallowed = files.filter((f) => !isAllowedResourceFilename(f.name)); + if (disallowed.length > 0) { + this.error.set( + `${disallowed.map((f) => f.name).join(', ')} ${DISALLOWED_RESOURCE_MESSAGE}`, + ); + return; + } if (this.isEditMode()) { const id = this.skillId()!; this.resourceBusy.set(true); diff --git a/frontend/ai.client/src/app/components/pulsating-loader.component.spec.ts b/frontend/ai.client/src/app/components/pulsating-loader.component.spec.ts new file mode 100644 index 000000000..a2eda3901 --- /dev/null +++ b/frontend/ai.client/src/app/components/pulsating-loader.component.spec.ts @@ -0,0 +1,59 @@ +import { Component } from '@angular/core'; +import { TestBed } from '@angular/core/testing'; +import { describe, it, expect } from 'vitest'; +import { PulsatingLoaderComponent } from './pulsating-loader.component'; + +@Component({ + imports: [PulsatingLoaderComponent], + template: ``, +}) +class HostComponent { + notice: string | null = null; +} + +function render(notice: string | null) { + const fixture = TestBed.createComponent(HostComponent); + fixture.componentInstance.notice = notice; + fixture.detectChanges(); + return fixture; +} + +describe('PulsatingLoaderComponent', () => { + it('cycles its own phrases when no notice is set', () => { + const fixture = render(null); + const loader = fixture.debugElement.children[0].componentInstance as PulsatingLoaderComponent; + // The typewriter starts empty and fills in; what matters is that it is not + // showing a caller-supplied string. + expect(loader.displayText()).not.toContain('Retrying'); + }); + + it('shows the notice verbatim instead of a loading phrase', () => { + const fixture = render('The model is busy. Retrying…'); + const text = (fixture.nativeElement as HTMLElement).textContent ?? ''; + expect(text).toContain('The model is busy. Retrying'); + }); + + it('drops the typing cursor for a notice', () => { + // The cursor reads as "still typing" on text that is finished. + const withNotice = render('Still working…'); + expect((withNotice.nativeElement as HTMLElement).querySelector('.typing-cursor')).toBeNull(); + + const withoutNotice = render(null); + expect( + (withoutNotice.nativeElement as HTMLElement).querySelector('.typing-cursor'), + ).not.toBeNull(); + }); + + it('marks the indicator dot so the change is visible peripherally', () => { + const fixture = render('Still working…'); + const dot = (fixture.nativeElement as HTMLElement).querySelector('.pulsing-circle'); + expect(dot?.classList.contains('is-notice')).toBe(true); + }); + + it('announces a notice to assistive tech', () => { + const fixture = render('Still working…'); + const status = (fixture.nativeElement as HTMLElement).querySelector('[role="status"]'); + expect(status?.getAttribute('aria-live')).toBe('polite'); + expect(status?.getAttribute('aria-label')).toContain('Still working'); + }); +}); diff --git a/frontend/ai.client/src/app/components/pulsating-loader.component.ts b/frontend/ai.client/src/app/components/pulsating-loader.component.ts index ccf1c395f..365e31a0c 100644 --- a/frontend/ai.client/src/app/components/pulsating-loader.component.ts +++ b/frontend/ai.client/src/app/components/pulsating-loader.component.ts @@ -3,6 +3,7 @@ import { ChangeDetectionStrategy, signal, computed, + input, OnInit, OnDestroy, } from '@angular/core'; @@ -41,9 +42,15 @@ const LOADING_PHRASES = [ * loading phrases, typing in character by character, pausing, then deleting * before showing the next phrase. * + * When `notice` is set, the playful cycling stops and the loader states a + * specific fact instead — used when the backend is retrying a failed model + * call. A retry is otherwise indistinguishable from a hang, and cheerful + * phrases like "Pondering..." during a provider outage actively mislead. + * * @example * ```html * + * * ``` */ @Component({ @@ -54,20 +61,30 @@ const LOADING_PHRASES = [ class="flex items-center gap-4" role="status" [attr.aria-busy]="true" + [attr.aria-live]="notice() ? 'polite' : null" [attr.aria-label]="'Loading: ' + displayText()" > - + - +
- + {{ displayText() }} - + @if (!notice()) { + + }
@@ -111,6 +128,18 @@ const LOADING_PHRASES = [ animation: pulse-dot 1.25s cubic-bezier(0.455, 0.03, 0.515, 0.955) -0.4s infinite; } + /* Notice state: same pulse, different signal colour. The dot is the + only thing a user tracks peripherally, so it has to change too — + swapping just the text leaves the indicator looking routine. */ + .pulsing-circle.is-notice::before, + .pulsing-circle.is-notice::after { + background-color: var(--color-amber-500); + } + + .pulsing-circle.is-notice::after { + box-shadow: 0 0 8px var(--color-amber-500); + } + @keyframes pulse-ring { 0% { transform: scale(0.33); @@ -159,6 +188,12 @@ const LOADING_PHRASES = [ `, }) export class PulsatingLoaderComponent implements OnInit, OnDestroy { + /** + * Fixed message that replaces the cycling phrases, e.g. a retry in + * progress. Null (the default) keeps the normal typewriter behaviour. + */ + notice = input(null); + // Base timing constants (in milliseconds) private readonly TYPE_SPEED_BASE = 45; private readonly TYPE_SPEED_VARIANCE = 35; @@ -181,8 +216,14 @@ export class PulsatingLoaderComponent implements OnInit, OnDestroy { // Timer reference for cleanup private animationTimer: ReturnType | null = null; - // Computed display text with ellipsis + // Computed display text with ellipsis. A notice wins outright; the + // typewriter keeps ticking underneath so it resumes cleanly when the + // notice clears mid-turn. displayText = computed(() => { + const notice = this.notice(); + if (notice) { + return notice; + } const phrase = LOADING_PHRASES[this.currentPhraseIndex()] + '...'; return phrase.substring(0, this.currentCharIndex()); }); diff --git a/frontend/ai.client/src/app/my-skills/my-skill-form.page.html b/frontend/ai.client/src/app/my-skills/my-skill-form.page.html index 5a2376b11..23c52b776 100644 --- a/frontend/ai.client/src/app/my-skills/my-skill-form.page.html +++ b/frontend/ai.client/src/app/my-skills/my-skill-form.page.html @@ -161,6 +161,7 @@

Tools this skill id="resource-upload" type="file" multiple + [attr.accept]="acceptedFileTypes" class="sr-only" [disabled]="uploading()" (change)="onFilesSelected($event)" diff --git a/frontend/ai.client/src/app/my-skills/my-skill-form.page.ts b/frontend/ai.client/src/app/my-skills/my-skill-form.page.ts index 5cf2011f0..fd1493c32 100644 --- a/frontend/ai.client/src/app/my-skills/my-skill-form.page.ts +++ b/frontend/ai.client/src/app/my-skills/my-skill-form.page.ts @@ -9,6 +9,11 @@ import { heroTrash, } from '@ng-icons/heroicons/outline'; import { parseSkillMarkdown } from '../admin/skills/models/skill-import.util'; +import { + DISALLOWED_RESOURCE_MESSAGE, + RESOURCE_ACCEPT_ATTR, + isAllowedResourceFilename, +} from '../shared/skills/skill-resource-types'; import { MAX_RESOURCE_BYTES, MAX_RESOURCES_PER_SKILL, @@ -45,6 +50,8 @@ export class MySkillFormPage { protected readonly resourceKinds = RESOURCE_KINDS; protected readonly maxFiles = MAX_RESOURCES_PER_SKILL; + /** `accept` filter for the file picker — the allowed extensions. */ + protected readonly acceptedFileTypes = RESOURCE_ACCEPT_ATTR; protected readonly skillId = signal(null); protected readonly isEdit = computed(() => this.skillId() !== null); @@ -163,6 +170,18 @@ export class MySkillFormPage { ); return; } + // Mirror of the backend type allowlist. The server is the control (it + // rejects these with a 400); this just turns that into an immediate, + // specific message instead of a failed round-trip. Web-document types are + // refused because a resource is downloaded by other users from this app's + // own origin, where a rendered document could run script in their session. + const disallowed = files.filter((f) => !isAllowedResourceFilename(f.name)); + if (disallowed.length > 0) { + this.error.set( + `${disallowed.map((f) => f.name).join(', ')} ${DISALLOWED_RESOURCE_MESSAGE}`, + ); + return; + } if (this.fileCount() + files.length > this.maxFiles) { this.error.set(`A skill can hold at most ${this.maxFiles} supporting files.`); return; diff --git a/frontend/ai.client/src/app/session/components/message-list/message-list.component.html b/frontend/ai.client/src/app/session/components/message-list/message-list.component.html index f7a57f9c6..3a3e11fa6 100644 --- a/frontend/ai.client/src/app/session/components/message-list/message-list.component.html +++ b/frontend/ai.client/src/app/session/components/message-list/message-list.component.html @@ -147,7 +147,7 @@ @if (isChatLoading()) {
- +
} diff --git a/frontend/ai.client/src/app/session/components/message-list/message-list.component.ts b/frontend/ai.client/src/app/session/components/message-list/message-list.component.ts index 9ddeb211e..f6b544ca0 100644 --- a/frontend/ai.client/src/app/session/components/message-list/message-list.component.ts +++ b/frontend/ai.client/src/app/session/components/message-list/message-list.component.ts @@ -1,4 +1,4 @@ -import { Component, computed, input, output, inject, PLATFORM_ID } from '@angular/core'; +import { Component, computed, effect, input, output, inject, signal, PLATFORM_ID } from '@angular/core'; import { isPlatformBrowser, NgTemplateOutlet } from '@angular/common'; import { Message } from '../../services/models/message.model'; import type { Artifact } from '../../services/artifacts/artifact.model'; @@ -27,6 +27,7 @@ import { } from '../../../services/tool-approval/tool-approval.service'; import { CompactionSummaryService } from '../../services/chat/compaction-summary.service'; import { ChatStateService } from '../../services/chat/chat-state.service'; +import { StreamParserService } from '../../services/chat/stream-parser.service'; @Component({ selector: 'app-message-list', @@ -57,6 +58,27 @@ export class MessageListComponent { private readonly HEADER_HEIGHT = 64; private readonly SCROLL_PADDING = 16; + /** + * Silence thresholds for the "still working" notice. + * + * A model call that produces nothing looks exactly like a hung one. In prod + * session 5f34d2b0 two turns went ~95 seconds with no output and the user + * abandoned both — the second one while the request was, as far as the + * telemetry shows, still in flight. + * + * 30s is comfortably past a normal first token (~5-7s) and past most tool + * calls, so a healthy turn rarely trips it. 90s is past anything routine and + * is where the phrasing stops reassuring and starts admitting something is + * wrong. The tick is coarse because the thresholds are: the notice appears + * within one tick of crossing them. + */ + private readonly STALL_NOTICE_MS = 30_000; + private readonly LONG_STALL_NOTICE_MS = 90_000; + private readonly STALL_TICK_MS = 5_000; + + /** Clock for the stall thresholds; only ticks while a response is pending. */ + private readonly nowMs = signal(Date.now()); + messages = input.required(); isChatLoading = input(false); streamingMessageId = input(null); @@ -83,6 +105,84 @@ export class MessageListComponent { private artifactState = inject(ArtifactStateService); private mcpAppCardState = inject(McpAppCardStateService); private chatStateService = inject(ChatStateService); + private streamParser = inject(StreamParserService); + + /** + * Copy for the loading indicator while the backend retries a failed model + * call. Null during a normal response, which leaves the usual cycling + * phrases in place. Read straight off the parser by session id rather than + * threaded through every `[isChatLoading]` binding — preview and test-drive + * hosts have no session id and correctly get nothing. + */ + constructor() { + // Tick only while a response is pending: an always-on interval would wake + // every open conversation forever to answer a question nobody is asking. + // The write happens in the callback, not the effect body, so this never + // re-triggers itself. + if (this.isBrowser) { + effect((onCleanup) => { + if (!this.isChatLoading()) { + return; + } + const timer = setInterval(() => this.nowMs.set(Date.now()), this.STALL_TICK_MS); + onCleanup(() => clearInterval(timer)); + }); + } + } + + /** + * Copy for a response that has gone quiet for long enough to look broken. + * + * Deliberately client-side. The server cannot say anything during a stalled + * model call without racing the agent stream against a timer, and that + * machinery — cancellation, lease release, interrupted-turn persistence — + * has already been the source of several production bugs. The SPA has the + * one fact that matters: when the last byte arrived. If the connection had + * dropped, fetch-event-source would have surfaced an error instead of + * silence, so silence on an open stream really is "the server has not sent + * anything yet". + */ + protected readonly stallNotice = computed(() => { + const sessionId = this.sessionId(); + if (!sessionId || !this.isChatLoading()) { + return null; + } + const lastEventAt = this.streamParser.lastEventAtFor(sessionId)(); + if (!lastEventAt) { + return null; + } + const silentFor = this.nowMs() - lastEventAt; + if (silentFor >= this.LONG_STALL_NOTICE_MS) { + return 'Still working \u2014 this is taking longer than usual.'; + } + if (silentFor >= this.STALL_NOTICE_MS) { + return 'Still working\u2026'; + } + return null; + }); + + protected readonly retryNotice = computed(() => { + const sessionId = this.sessionId(); + if (!sessionId) { + return null; + } + const retry = this.streamParser.modelRetryFor(sessionId)(); + if (!retry) { + return null; + } + return retry.attempt === 1 + ? 'The model is busy. Retrying\u2026' + : `The model is busy. Retrying \u2014 attempt ${retry.attempt}.`; + }); + + /** + * What the loading indicator says. A retry is a specific, known fact and + * outranks the generic stall notice, which is only ever an inference from + * elapsed silence. + */ + protected readonly loaderNotice = computed( + () => this.retryNotice() ?? this.stallNotice(), + ); /** Persisted app-initiated tool cards, hydrated on reload (PR #6). */ protected mcpAppCards = this.mcpAppCardState.cards; diff --git a/frontend/ai.client/src/app/session/services/chat/stream-liveness.spec.ts b/frontend/ai.client/src/app/session/services/chat/stream-liveness.spec.ts new file mode 100644 index 000000000..83f85619a --- /dev/null +++ b/frontend/ai.client/src/app/session/services/chat/stream-liveness.spec.ts @@ -0,0 +1,105 @@ +// stream-liveness.spec.ts +// +// A model call that produces nothing looks exactly like a hung one. In prod +// session 5f34d2b0 two turns went ~95 seconds with no output and the user +// abandoned both — the second one while the request was, as far as the +// telemetry shows, still in flight. The SPA holds the one fact that can tell +// those apart: when the last byte arrived. +import { TestBed } from '@angular/core/testing'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { StreamParserService } from './stream-parser.service'; +import { ChatStateService } from './chat-state.service'; +import { ErrorService } from '../../../services/error/error.service'; +import { QuotaWarningService } from '../../../services/quota/quota-warning.service'; + +describe('StreamParserService - stream liveness', () => { + let service: StreamParserService; + + beforeEach(() => { + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + providers: [StreamParserService, ChatStateService, ErrorService, QuotaWarningService], + }); + service = TestBed.inject(StreamParserService); + }); + + afterEach(() => { + vi.useRealTimers(); + TestBed.resetTestingModule(); + }); + + it('reports 0 before a stream has started', () => { + expect(service.lastEventAtFor('never-started')()).toBe(0); + }); + + it('stamps the clock when a stream is reset', () => { + const before = Date.now(); + service.reset('s1'); + expect(service.lastEventAtFor('s1')()).toBeGreaterThanOrEqual(before); + }); + + it('advances on each received event', () => { + vi.useFakeTimers(); + service.reset('s1'); + const first = service.lastEventAtFor('s1')(); + + vi.advanceTimersByTime(10_000); + service.parseEventSourceMessage('s1', 'message_start', { role: 'assistant' }); + + expect(service.lastEventAtFor('s1')()).toBe(first + 10_000); + }); + + it('advances on an event the state gate drops', () => { + // Liveness is about the connection, not the payload. An event this stream + // state ignores is still proof the server is talking. + vi.useFakeTimers(); + service.reset('s1'); + service.parseEventSourceMessage('s1', 'done', {}); + const afterDone = service.lastEventAtFor('s1')(); + + vi.advanceTimersByTime(5_000); + service.parseEventSourceMessage('s1', 'metadata', { usage: {} }); + + expect(service.lastEventAtFor('s1')()).toBe(afterDone + 5_000); + }); + + it('advances on a malformed event', () => { + vi.useFakeTimers(); + service.reset('s1'); + const first = service.lastEventAtFor('s1')(); + + vi.advanceTimersByTime(3_000); + service.parseEventSourceMessage('s1', 'content_block_delta', { nonsense: true }); + + expect(service.lastEventAtFor('s1')()).toBe(first + 3_000); + }); + + it('ignores events from a superseded stream', () => { + // Otherwise a dead stream's late events would keep its replacement + // looking alive, and the replacement's own stall would never surface. + vi.useFakeTimers(); + service.reset('s1'); + const staleStreamId = service.getCurrentStreamId('s1'); + + service.reset('s1'); // new stream for the same session + const afterReset = service.lastEventAtFor('s1')(); + + vi.advanceTimersByTime(20_000); + service.parseEventSourceMessage('s1', 'message_start', { role: 'assistant' }, staleStreamId); + + expect(service.lastEventAtFor('s1')()).toBe(afterReset); + }); + + it('tracks each session independently', () => { + vi.useFakeTimers(); + service.reset('s1'); + service.reset('s2'); + const s1Start = service.lastEventAtFor('s1')(); + + vi.advanceTimersByTime(30_000); + service.parseEventSourceMessage('s2', 'message_start', { role: 'assistant' }); + + expect(service.lastEventAtFor('s1')()).toBe(s1Start); + expect(service.lastEventAtFor('s2')()).toBe(s1Start + 30_000); + }); +}); diff --git a/frontend/ai.client/src/app/session/services/chat/stream-parser.service.ts b/frontend/ai.client/src/app/session/services/chat/stream-parser.service.ts index 3b99cf318..a002fb6ae 100644 --- a/frontend/ai.client/src/app/session/services/chat/stream-parser.service.ts +++ b/frontend/ai.client/src/app/session/services/chat/stream-parser.service.ts @@ -30,6 +30,7 @@ import type { UiResourceEvent, ToolInputPartialEvent, SessionTitleEvent, + ModelRetryEvent, } from '../../../shared/utils/stream-parser'; import { processStreamEvent, @@ -96,6 +97,24 @@ interface ParserSessionState { /** Tool progress indicator state */ toolProgress: WritableSignal; + /** + * Epoch ms of the last event received on this stream. Stamped on EVERY + * event, including ones the state gate then drops — the point is liveness + * of the connection, not whether the payload was useful. Lets the UI tell a + * long stall apart from a hang, which is otherwise impossible from the + * client side (prod 5f34d2b0: two turns went ~95s with no output and the + * user abandoned both). + */ + lastEventAt: WritableSignal; + + /** + * The most recent model-call retry this turn, or null. Set by the + * `model_retry` SSE event and cleared as soon as content arrives, so the + * loading indicator can explain the silence instead of leaving the user to + * read it as a hang. + */ + modelRetry: WritableSignal; + /** Error state */ error: WritableSignal; @@ -152,6 +171,8 @@ export class StreamParserService { private readonly allMessagesCache = new Map>(); private readonly streamingMessageIdCache = new Map>(); private readonly toolProgressCache = new Map>(); + private readonly modelRetryCache = new Map>(); + private readonly lastEventAtCache = new Map>(); private readonly citationsCache = new Map>(); private readonly errorCache = new Map>(); private readonly isStreamCompleteCache = new Map>(); @@ -182,6 +203,23 @@ export class StreamParserService { return this.cachedAccessor(this.toolProgressCache, sessionId, (state) => state.toolProgress(), { visible: false }); } + /** + * The current model-call retry notice for a session, or null when the model + * is responding normally. Drives the "still working" copy on the loader. + */ + modelRetryFor(sessionId: string): Signal { + return this.cachedAccessor(this.modelRetryCache, sessionId, (state) => state.modelRetry(), null); + } + + /** + * Epoch ms of the last event seen on a session's stream, or 0 before one + * starts. The UI compares it against the clock to decide when silence has + * gone on long enough to be worth explaining. + */ + lastEventAtFor(sessionId: string): Signal { + return this.cachedAccessor(this.lastEventAtCache, sessionId, (state) => state.lastEventAt(), 0); + } + /** Pending citations for a session's next assistant message. */ citationsFor(sessionId: string): Signal { return this.cachedAccessor(this.citationsCache, sessionId, (state) => state.pendingCitations(), []); @@ -203,7 +241,15 @@ export class StreamParserService { */ parseSSELine(sessionId: string, line: string): void { const state = this.states().get(sessionId); - if (!state || !this.shouldProcessEvent(state)) { + if (!state) { + return; + } + + // Liveness first — see parseEventSourceMessage. A line the state gate + // below drops still proves the connection is alive. + state.lastEventAt.set(Date.now()); + + if (!this.shouldProcessEvent(state)) { return; } @@ -235,6 +281,12 @@ export class StreamParserService { return; // Stale event from a superseded stream for this session. } + // Stamp liveness before the validation and state gates below: a `ping` or + // an event this stream state drops is still proof the server is talking. + // Placed after the stale-stream guard so a superseded stream can't keep + // its replacement looking alive. + state.lastEventAt.set(Date.now()); + // Validate inputs if (!event || typeof event !== 'string') { this.setError(state, 'parseEventSourceMessage: event must be a non-empty string'); @@ -346,6 +398,8 @@ export class StreamParserService { currentMessageBuilder, completedMessages, toolProgress: signal({ visible: false }), + modelRetry: signal(null), + lastEventAt: signal(Date.now()), error: signal(null), isStreamComplete, metadata: signal(null), @@ -433,6 +487,13 @@ export class StreamParserService { onToolResult: (data) => this.handleToolResult(state, data), onToolProgress: (progress) => state.toolProgress.set(progress), + onModelRetry: (data: ModelRetryEvent) => { + // Not viewed-session-scoped on purpose: this signal is read per + // session id, so a background conversation's retry stays with that + // conversation instead of leaking into the one on screen. + state.modelRetry.set(data); + }, + onMetadata: (data) => this.handleMetadata(state, data), onReasoning: (data) => this.handleReasoning(state, data), onCitation: (data) => this.handleCitation(state, data), @@ -572,6 +633,9 @@ export class StreamParserService { // Clear any previous errors state.error.set(null); + // Content is arriving, so whatever retry we were explaining is over. + state.modelRetry.set(null); + // If there's an existing message, finalize it before starting a new one const currentBuilder = state.currentMessageBuilder(); if (currentBuilder) { @@ -826,6 +890,7 @@ export class StreamParserService { this.finalizeCurrentMessage(state); state.isStreamComplete.set(true); state.toolProgress.set({ visible: false }); + state.modelRetry.set(null); state.streamState = StreamState.Completed; // Automatic cleanup after delay. Guarded on the stream ID so a session diff --git a/frontend/ai.client/src/app/shared/skills/skill-resource-types.spec.ts b/frontend/ai.client/src/app/shared/skills/skill-resource-types.spec.ts new file mode 100644 index 000000000..55625c9cb --- /dev/null +++ b/frontend/ai.client/src/app/shared/skills/skill-resource-types.spec.ts @@ -0,0 +1,64 @@ +/** + * Guards the client mirror of the skill-resource type allowlist. + * + * The server is the real control, but this mirror decides what the file picker + * offers and what the forms refuse locally — so it must never drift open on the + * dangerous extensions. The stored-XSS chain these tests exist for: a + * `.html` skill resource served from the SPA's own origin executed its inline + * `