Backmerge: main into develop (1.19.0) - #992
Merged
Merged
Conversation
* feat(marketplace): submitting an agent makes it public, with a notice
The submit dialog asked the author to tick "Make this agent public" and
held Submit until they did. The reasoning was that widening access must
be consented to, and that reasoning holds — but the control did not serve
it. The store is one public shelf, so there is no submission that leaves
an agent private: the box had exactly one valid answer, and a required
control with one valid answer is ceremony that trains people to click
past it rather than consent.
Replaced with a disclosure above the form — "Submitting makes this agent
public", worded for what it changes *from* (private vs shared) and saying
that the widening happens now rather than at approval, which is the part
an author would otherwise be surprised by. Pressing Submit under that
notice is the consent.
The backend gate is deliberately unchanged: `_visibility_block` still
refuses a submission whose request omits `makePublic`, so a direct API
caller — who sees no notice — cannot widen an agent by accident. The
dialog is where the notice is shown, so the dialog is where the flag is
now always set.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(marketplace): let admins read, test-drive and decline a submission
A reviewer could name a submission but not see one. Three individually
correct access rules produced that: `instructions` is gated to
owner/editor on `GET /agents/{id}`; that read refuses a non-owner
outright on a PRIVATE agent, and a PRIVATE agent can sit in the queue;
and the review diff — the only other window onto the content — is empty
by construction on a first submission. So on the most consequential
review in the system, the admin saw a name, an author and a category.
Three additions.
**Read it.** `GET /admin/agents/{id}/submission` returns instructions,
bound capabilities by name, model, starters, publisher and reachability,
rendered by a new page at `/admin/marketplace/review/:agentId`. It serves
the frozen snapshot, never the live record: the live record is the
author's draft and they can keep editing while the row sits in the queue,
while approval promotes `submittedVersion`. Reading anything else shows
an admin one configuration and publishes another — the window
`AgentVersion` exists to close. Deliberately a separate endpoint rather
than a widened `GET /agents/{id}`, which is the store's detail read *and*
the Designer's form loader, and would serve the wrong version anyway.
**Test-drive it.** `review_preview` on the invocation payload resolves the
reviewed snapshot and bypasses the PRIVATE check, after re-resolving
`admin.marketplace` against the caller's own roles — a caller without it
is refused rather than quietly downgraded, since a silent downgrade would
run the published snapshot and report it as the version under review. The
which-version rule lives in `version_resolution.resolve_review_agent` so
the page and the test drive cannot disagree; inference-api cannot import
from app_api, and two copies of that rule is the same class of bug one
level up. Runs on a `preview-` session and skips the path's bookkeeping
writes — a reviewer poking a submission is not use.
**Decline it.** A third decision, not a harsher request-changes: an admin
who judged a submission not a fit had to publish it or say "fix this",
promising a review they did not intend to give. `rejected` is admin-only
from `in_review`, carries a required reason, and still lets the author
revise and resubmit. It is never on the shelf and has no edge to
`published`, so approval remains the only door into the store;
`rejected -> private` exists so a declined author can still delete their
own agent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(marketplace): make the review test drive big enough to use
The panel was sized by its grid cell, so its height came from whatever the
prose in the left column happened to need. On a short submission that left
about 120px between the warning banner and the composer — too small a
window to judge an agent through, which is the one thing the panel exists
for.
Size it against the viewport instead, and make it sticky so it stays put
while the reviewer scrolls the instructions: reading and asking are one
loop, not two passes. Add an expand control that spans it across both
columns for a longer session. Expanding is a class change only — the
element never moves in the DOM, because remounting would destroy the
component and take the reviewer's conversation with it.
Also: an empty summary or system prompt now says so instead of rendering a
blank box. A blank panel under a heading reads as a page that failed to
load, and an agent that genuinely ships no instructions is itself a reason
to decline — the two must not look alike.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(kb): managed knowledge base migration — inert behind flags, upgrade UX live in dev (#884)
* docs(kb): §13 benchmark outcome + production baseline measurements
Records the §13 benchmark results and adds a production measurement pass that
replaces the earlier hypothetical cost and volume figures.
Benchmark writeup (§5.1, §11, §12, §13.5, §13.6, §14.0):
- revised latency and ingest figures from the 9-question x 3-backend harness
- gate outcome (CLEARED, 4 of 5 conditions), context-cap experiment, and the
gaps AWS has since closed
- unrelated prod/dev bugs and debris surfaced while benchmarking
Production baseline, prod 897729136999 (§3.1, §3.2, §3.3, §6.4, §7.4, §13.5):
- 3,886 retrievals/30d, ~26% RAG attach rate, $6.54/mo like-for-like against
~$0.09/mo today; scaling table out to 30,000 users
- reconciles the expected-value trajectory ($169/mo) against §13.5's policy
ceiling ($150,000/mo) so the former cannot be used to argue against the cap
- $5.00/GB verified to meter raw source bytes, not post-parse text
- adds the missing agentic-retrieval and Gateway line items to §3.1
- no seasonality baseline exists: all six recorded months are adoption ramp,
with no summer trough (Aug pacing 1.7x Jul)
- quota audit vs measured scale; query input is capped at 10,000 chars and is
not adjustable, the only finding that hard-fails rather than costing money
- agentic RPM is 60 per *account*, not per KB
- document status distribution: 200 of 1,692 non-complete; the status filter
is correct, but fails open on DynamoDB error and so can serve chunks from
deleted documents
- new §13.5 requirement 4: clamp query length before Retrieve (gate count 3->4)
§6.5 proposes agentic retrieval as a user-triggered per-answer escalation,
which keeps the 60 RPM account quota non-binding below ~30% escalation.
* docs(kb): clarify attachment/KB composition; de-duplicate fail-open defect
§6.3 — attachments and knowledge bases compose in one turn:
- states explicitly that keeping attachments out of Managed KB does not exclude
using both together; the inline document block and retrieved chunks already
reach the model in the same prompt (prompt_builder, augment_prompt_with_context,
merged at inference_api/chat/routes.py:2234)
- adds the privacy argument for the decision, which previously rested only on
latency and whole-document reasoning: an attachment ingested into a shared
agent's KB becomes retrievable by every other user of that agent
- flags that the 2,000-char context cap is the real constraint for this shape of
task, and that §13.6's "the cap costs nothing" result covers single-fact lookups
only; nominates compare-and-contrast as the question set for the multi-chunk
synthesis experiment §13.6 says is missing
- notes retrieval returns fragments, never a whole exemplar, with two unproven
leads (GetDocumentContent; agentic retrieval, which is not cap-bound)
§7.4 / §11.1 / §14.4 — the fail-open document-status filter was described three
times, in three levels of detail:
- §7.4 is now canonical and marked as such
- §11.1 reduced to a one-line comparison against managed ACL behaviour, keeping
its own distinct points (ACL awareness is not authorization; email-only identity)
- §14.4 keeps the requirement and references §7.4 for the evidence
- adds a scope note that was previously only implicit: the inner per-document
handler already fails closed, so only the outer table-level handler fails open.
The exposure window is table unavailability, not ordinary throttling — low
probability, privacy-grade consequence, one-line fix.
Docs only. No code or infrastructure change.
* feat(kb): managed knowledge base migration — spec, schema and worker platform
Introduces Amazon Bedrock Managed Knowledge Base as a second retrieval backend
behind an abstraction seam. This lands the spec plus task groups 1 and 2: the
additive schema, the IAM, and the worker resources. All of it is dormant.
Scope is evaluation §14.7 phases 1-4 (additive schema/IAM, dark dual backends,
opt-in dual-read pilot, opt-in migration with a rollback window). Phases 5-8 are
a follow-up spec.
Group 1 — additive schema and IAM
* KbWorkIndex GSI (GSI7_PK/GSI7_SK) on the assistants table, sparse: keys exist
only while a knowledge base is eligible for background work, so an ineligible
or pinned KB is invisible to the dispatcher by physics rather than by filter.
* GSI7_* added to the generic assistant-update immutable-field guard.
* Bedrock KB service role with confused-deputy conditions (aws:SourceAccount +
ArnLike AWS:SourceArn on knowledge-base/*), S3 read conditioned on
aws:ResourceAccount, and bedrock:InvokeModel pinned to
amazon.titan-embed-text-v2:0 only.
* Caller grants split by need: provisioning CRUD, direct ingestion, and
inference Retrieve, each under its own SID; iam:PassRole scoped to the service
role ARN and conditioned on iam:PassedToService.
Group 2 — worker resources and config
* Dispatcher, worker, reconciler and ingestion consumer as four Lambdas sharing
ONE Docker image, with a byte-stable bootstrap stub per the
platform-as-bootstrap pattern. The real image ships via the workflow later.
* Ingestion consumer at 900s with a dead-letter queue; a 50 KiB PDF was measured
at 264s, so anything under 300s would turn a slow success into a retry storm.
* Three independent opt-in flags, all defaulting OFF: managed-by-default,
migration enabled, and reconciler armed. An unset GitHub Actions variable
renders as an empty string, which parseBooleanEnv maps to false.
* Per-owner byte caps and four fleet-level alarms, all NOT_BREACHING on missing
data. Per-owner caps bound one user; the alarms bound the account.
DEPLOY ORDER CONSTRAINT: this consumes the entire rag-assistants GSI budget for
whichever release ships it. DynamoDB permits exactly one GSI create/delete per
UpdateTable and CloudFormation issues one per changed table, so a release adding
a second index to that table fails and rolls back — the 1.12.0 outage shape. Any
other in-flight change adding a GSI to rag-assistants must ship separately.
Two defects in the spec were found and corrected while implementing it:
* PutMetricData was specified against AWS/Bedrock/KnowledgeBases. CloudWatch
reserves every namespace beginning with "AWS" and rejects writes to them, so
the grant would have deployed cleanly and published nothing. Custom metrics now
target ${projectPrefix}/ManagedKb; Bedrock's own namespace is a read source.
* The same grant was specified on the Bedrock service role, which Bedrock
assumes and which never publishes our metrics. Removed as a dead grant.
Zero behaviour change: 271 -> 307 resources with none removed, no IAM statement
lost, no existing logical ID renamed, and the documents-bucket notification
preserved byte-for-byte. Verified by baseline-vs-current synth.
Tests: 611 infrastructure tests green. Security-relevant assertions are
mutation-verified — each was confirmed to fail with its guard removed, which
caught three tests that had been passing vacuously.
* feat(kb): KB_Record data layer with conditional state transitions
Task group 3 of the managed knowledge base migration. Adds the control-plane
record for a knowledge base and the six state transitions the migration worker
drives, in a new apis.shared.kb_backend package. Nothing calls it yet.
Three invariants are enforced here rather than left to callers, because each one
fails silently when violated:
* Absence means legacy. `retrievalEngine` is only ever written as "managed";
nothing writes "s3vectors" onto a record that lacked it. That is what makes the
migration zero-backfill — 1,692 existing records are already correct by having
no opinion — and what makes rollback a REMOVE rather than a rewrite.
* Every transition is conditional. These run from a dispatcher that fans out to
concurrent workers, so a read-then-write would let two workers both believe
they won. Losing a race surfaces as TransitionLost, distinct from a real error.
* Sparse work keys are REMOVED on reaching a terminal state, not merely ignored.
A stale GSI7 key hands the dispatcher a knowledge base nobody asked to migrate,
and the dispatcher creates and deletes billed AWS resources.
The package talks to DynamoDB through the raw table resource instead of importing
apis.shared.assistants, whose __init__ pulls the embeddings stack in at module
scope and would blow the migration Lambda's image budget — the same reason
kb_sync/records.py is written this way. Module-level imports are stdlib only and
boto3 is function-local. Verified empirically: importing records.py in a fresh
interpreter loads no assistants or embeddings module, and does not load boto3 at
all. The formal architecture test lands with task 4.8.
Bug found by the tests: `total` is a DynamoDB reserved keyword, so the converged
catch-up guard comparing migrationProgress.migrated to .total was rejected
outright. Now aliased through ExpressionAttributeNames. It failed loudly rather
than silently, but it would have broken every promotion.
Tests: 26 transition tests against moto plus 9 hypothesis property tests, all
mutation-verified — each of the 10 guards was removed in turn and confirmed to
break a specific test.
Note on that verification, because it nearly went wrong: the first mutation round
reported all 10 guards caught, but four were false passes. Removing a
ConditionExpression orphans the expression values it referenced, DynamoDB rejects
a request carrying unused values, and the resulting ValidationException turned the
HAPPY-PATH test red instead of the guard test. Re-running those four with the
orphaned references stripped — so the write genuinely succeeds without its guard —
put the failure on the correct tests. "A test failed" is not "the mutation was
caught"; which test failed is the evidence.
* refactor(kb): backend abstraction seam behind the retrieval entry point
Task group 4. Puts a protocol between the retrieval entry point and the vector
store so a second backend can be added later without the callers knowing. No
behaviour change: same results, same ordering, same response keys, same failure
modes.
* protocol.py — KnowledgeBaseBackend Protocol plus a frozen Chunk whose score
field is `relevance`, higher-is-better.
* resolver.py — reads retrievalEngine from the KB_Record; absence resolves to the
legacy backend, reusing records.resolve_engine rather than reimplementing it.
* s3vectors_backend.py — the existing S3 Vectors path, moved without functional
change, converting distance to relevance inside the adapter.
* rag_service.py — now a facade that resolves then delegates. It also owns the
parity rules (top_k, the 2,000-character cap, the document-status filter, the
citation clip) so one implementation covers every backend.
SCORE DIRECTION is the highest-risk detail in this feature. S3 Vectors returns
cosine distance, lower-is-better; the seam speaks relevance, higher-is-better.
Invert it and nothing raises: retrieval still returns five chunks, every request
still succeeds, and the answers just quietly get worse. Conversion is by exact
negation, done once inside the adapter, and is order-preserving and losslessly
reversible — unlike 1-d or 1/(1+d). The facade still emits a `distance` key
derived from relevance, because app_api/assistants/routes.py puts that value in
an HTTP response body a client already reads.
The public contract is unchanged. The facade's runtime signature is still
(assistant_id, query, top_k=5) and it still returns text/distance/metadata/key.
Both call sites — inference_api/chat/routes.py and app_api/assistants/routes.py —
are byte-identical.
The document-status filter's two fail-open paths are PRESERVED deliberately: the
branch taken when DYNAMODB_ASSISTANTS_TABLE_NAME is unset, and the outer except.
Inverting them to fail closed is Requirement 5, task group 6. Doing it here would
bury a behaviour change inside a refactor. The previously-untested second path now
has a test pinning current behaviour, so that inversion will be a deliberate,
visible edit rather than a silent one.
Tests: 6032 backend tests pass, up 34. Mutation-verified, including the two that
matter most — inverting the score conversion fails four tests (ranking, agreement
between backends, direction, and round-trip exactness), and adding an
apis.shared.assistants import to a seam module fails the new architecture test.
Each mutation was confirmed to have actually landed via diff before its result was
trusted; two earlier attempts silently missed their anchor and would otherwise
have been recorded as passes.
* feat(kb): clamp retrieval queries and fail closed on unconfirmable status
Task groups 5 and 6. Group 6 is a deliberate BEHAVIOUR CHANGE to live retrieval;
group 5 is a new guard that also changes behaviour for pathologically long
queries. Both are called out below.
Group 5 — query clamp
Managed Knowledge Base caps `Retrieve` input at 10,000 characters and the quota is
not adjustable, so an over-long query is a rejected request rather than a worse
answer. The clamp is applied in the facade, before dispatch, so both backends
receive an identically-shaped query — clamping only the managed path would mean the
two backends answered different questions whenever a query ran long, which would
make a dual-read rank disagreement indistinguishable from a genuine retrieval
difference.
This does clamp the legacy path, which was previously unclamped: Titan v2 tolerates
roughly 32,000 characters, so queries between the two ceilings used to be embedded
whole. That is the intended trade — parity is worth more than the tail of a
pathological query — and it is why truncation emits KbQueryClamped rather than
passing silently. The clamp never raises; a long query is a fixable input, not a
failed chat turn.
Also removed two stale comments in bedrock_embeddings.py asserting that search
queries need no length validation. True only for Titan; not true for every backend.
Group 6 — fail-closed document status filter
Both table-level fallbacks in _filter_vectors_by_document_status now DROP chunks
instead of returning them unfiltered: the branch taken when
DYNAMODB_ASSISTANTS_TABLE_NAME is unset, and the outer exception handler. The
per-document handler already failed closed and is untouched — one unreadable
document should cost that document, not the whole result.
This supersedes reliable-document-deletion Requirement 3.4, which specified the
fail-open deliberately. That requirement is annotated as superseded rather than
silently contradicted. What changed is evidence, not taste: 936 retrievals in a
trailing 30-day window had chunks removed by this filter, so the documents it
guards are real. A lookup failure would therefore have served users content they
believe they deleted, with nothing in the response indicating the check was skipped.
Failures now log at ERROR and emit KbStatusFilterFailClosed, so an empty result from
this path is distinguishable from "the corpus had no match".
Tests: 6054 backend tests pass, up 22. Mutation-verified, and three mutations
initially SURVIVED a suite that looked thorough — worth recording because each was
a distinct blind spot:
* Raising the cap to 32,000 went unnoticed, because every test referenced
MAX_QUERY_CHARS symbolically and so followed the constant wherever it moved. Now
pinned to the literal 10,000, which is a property of AWS rather than a knob.
* Removing the clamp call from the facade went unnoticed: the unit properties all
still passed while the clamp became dead code. Now asserted by inspecting what
the backend actually received.
* Pointing the metric namespace at a reserved AWS/ one went unnoticed. That is the
same silently-inert-grant bug already fixed in the CDK layer; this is the backend
half of the assertion.
Also fixed a latent flake in the new property test: it imported the module under
test inside a patched context, so the first run in a session behaved differently
from every later one — failing cold and passing warm, which is the worst failure
mode for a guard given CI is always cold.
* feat(kb): per-owner byte cap with atomic reserve / commit / release
Task group 7. Managed storage is billed at $5.00/GB-month, roughly 35x what S3
Vectors costs today. At the measured 1.13 MB average per user that is about
$169/month across the fleet, but nothing structural stopped one user uploading far
more — 30,000 users at 100 MB each would be 3 TB, or ~$15,000/month. This is what
makes that impossible rather than merely unlikely.
The obvious implementation does not work. DynamoDB condition expressions compare
operands; they cannot do arithmetic, so
ConditionExpression="storedBytes + reservedBytes + :n <= :cap"
is rejected outright — verified: "Cannot parse condition starting at:+ reserved
<= :cap". The arithmetic therefore moves to the client, where it is free: a single
`totalBytes` accumulator is compared against a literal computed before the call
(`cap - n`). One atomic conditional ADD, so N concurrent reservations cannot
collectively overshoot. The read-compute-write alternative has a window between the
read and the write, which is exactly the race a cap exists to close.
Reserve / commit / release rather than a plain add, because ingestion is not
instantaneous — a 50 KiB PDF measured 68-264s. Counting only on success would let a
user start many concurrent uploads that are each under the cap and collectively far
over it. A crash between reserve and commit leaks a reservation, which is the safe
direction: it under-permits rather than over-permits.
Size always comes from an S3 HEAD, never a client-reported value. Bedrock's
RawDataSize is deliberately not used for enforcement: it returned 0 datapoints for a
directly-ingested document during evaluation, and enforcing against a metric that is
sometimes absent would fail open.
Requirement 12.10 (who consumes retrieval quota) is now decided and recorded in the
design. Half the answer is a fact about AWS: Managed KB's Retrieve quota is 600/min
PER KNOWLEDGE BASE, where S3 Vectors is 20 rps ACCOUNT-WIDE. So the owner is the
payer — and that is an improvement, not a compromise. Today one hot assistant can
exhaust an account-wide ceiling and degrade retrieval for everyone; per-KB quotas
make the blast radius one agent. The case to watch is a published agent, the only
realistic way to approach 600/min, currently running ~26/min on a hot shared
knowledge base.
Infra: app-api now receives the byte cap values and the metric namespace. It enforces
the cap on interactive upload, and without these it would have fallen back to module
defaults and silently ignored an operator's configured limits. The namespace comes
from the same helper that builds the IAM condition, so the grant and the publish
cannot drift.
Wiring the whole-snapshot reservation into the Migration_Worker is deferred to task
group 13, where that worker is built; `reserve_snapshot` and its tests land here so
the enforcement point exists before the caller does.
Tests: 6073 backend tests pass, up 19. All 11 mutations verified caught, including
removing the cap guard entirely, an off-by-one against the wrong bound,
double-counting on commit, leaking allowance on release, and trusting a
caller-supplied size. One mutation initially reported as caught was rejected on
inspection: it had produced a syntax error rather than a semantic detection, so it
was redone with the request left valid.
* feat(kb): managed knowledge base provisioning, retrieval and direct ingestion
Task group 8 — the first code that talks to Bedrock Managed Knowledge Base. Still
unreachable at runtime: nothing resolves to this backend until the flags are on.
Several request shapes here CONTRADICT the AWS documentation and were established by
direct probing. Each is asserted by a test so nobody "corrects" them back:
* CreateKnowledgeBase omits storageConfiguration entirely — there is no vector store
to provision, and sending it is rejected.
* The data source nests MANAGED_KNOWLEDGE_BASE_CONNECTOR with the real type inside
connectorParameters. Top-level CUSTOM/S3/WEB are all rejected for a MANAGED KB.
* Retrieval uses managedSearchConfiguration. vectorSearchConfiguration is rejected
outright. Hybrid search is not toggleable, so nothing tries.
* clientToken has a 33-character MINIMUM, and the natural "{id}-{variant}-kb" token
is 31 — it fails client-side, before any request is sent. Tokens are built, not
interpolated, and persisted so a retry reuses the same one.
* IngestKnowledgeBaseDocuments caps at 10 documents, server-enforced; the user
guide's 25 does not apply to managed knowledge bases.
* StartIngestionJob is never called: 0.1 RPS account-wide, not adjustable.
* "Unable to verify the specified embedding model" is retryable — pure IAM eventual
consistency, observed against a model confirmed ACTIVE and invokable. Treating it
as fatal would make lazy provisioning fail intermittently while blaming the model.
* imageExtractionStatus is set to ENABLED explicitly; left default, the knowledge
base silently indexes no image or chart content.
* dataDeletionPolicy is RETAIN, the documented remedy for the DELETE_UNSUCCESSFUL
state already present in the dev account.
The record is written in `provisioning` BEFORE the AWS call, so a crash after
CreateKnowledgeBase returns leaves a discoverable retry anchor rather than an orphan
nobody is billed for quietly. A test drives that ordering by reading DynamoDB from
inside the create call, and asserts a retry produces exactly one knowledge base.
Managed returns relevance natively (higher is better), so unlike the S3 Vectors
adapter this one applies NO conversion. A test asserts the scores pass through
unchanged, because an accidental inversion here raises nothing and merely makes
answers worse.
Two corrections to this spec, both found by implementing it and verified against the
packaged botocore service model rather than from memory:
* It said managedKnowledgeBaseConfiguration={}. The shape has no required members,
but its only members are the embedding pin and encryption — so a literal {} would
make Requirement 8.5's pin unsatisfiable. "No required members" is not "must be
empty".
* It said float32. The enum value is FLOAT32; lowercase is rejected.
Also bounded inline metadata attributes at 50, the service-model maximum. Caller
metadata was merged unbounded, and the blast radius is disproportionate: metadata is
per-document but the call is per-batch, so one over-decorated document would fail
the ingestion of the nine travelling with it. Reserved keys are emitted first,
because the previous plain sorted() order would have truncated away document_id —
the key the status filter joins on — and every chunk of that document would then be
discarded as unverifiable.
Tests: 6174 backend tests pass, up 101. No test contacts live AWS: both Bedrock
clients are hand-rolled fakes, DynamoDB is moto so the conditional writes execute for
real, and two tests walk the test file's own AST to enforce it. All mutations caught,
each verified to have landed and left the file syntactically valid first.
* feat(kb): ingestion consumer with exclusive engine routing
Task group 9. The Lambda that decides whether a newly uploaded document belongs to a
managed knowledge base and, if so, ingests it directly. Replaces nothing yet — the
bootstrap stub still ships until the real image is built.
The routing is deliberately ASYMMETRIC, and that is the whole point. The legacy
pipeline is driven by its own pre-existing S3 notification on the same bucket, so for
a legacy document the correct behaviour here is to do nothing at all. Acting anyway
would index the same bytes twice: two sets of vectors, doubled ingestion cost, and
duplicate chunks competing inside one result list. None of that raises an error,
which is exactly why it is tested from both sides — legacy must ingest nothing here,
and an unprovisioned managed knowledge base must FAIL rather than quietly fall back
and produce the dual index.
Indexed is not retrievable. Bedrock reports a document INDEXED up to a second before
it can actually be retrieved (measured 0.75-1.03s), so the consumer polls until a
real retrieval returns the document and records indexedAt and retrievableAt as two
distinct timestamps. Marking complete on INDEXED alone produces the worst kind of
bug report: the UI says the upload worked, the user asks immediately, and the answer
does not mention their document. A document that never becomes retrievable is left
non-terminal for redelivery rather than claiming a success the user cannot observe.
No in-process orchestration (Requirement 10.8): a background task is killed when the
handler returns, which turns a reported success into a half-finished ingestion.
Three defects found while writing this, each worth naming:
* I called the backend class ManagedBackend; it is ManagedKbBackend, and it takes the
App_KB_Id rather than the AWS identifiers because it resolves those itself on every
operation — a rehydration cycle replaces them and a caller holding a stale pair
would keep addressing a knowledge base that no longer exists. Reading the real
signatures rather than assuming them turned a runtime ImportError into a compile-
time fix.
* The "this module does not orchestrate in process" test grepped the source for
ensure_future and failed on the module's own docstring explaining why it does not
use ensure_future. Now parses the AST and inspects actual calls.
* The poll timeouts were default arguments, which Python binds once at import, so a
test shortening the window had no effect and silently waited the full production
timeout — 33 seconds of suite time for one test. They are now resolved at call
time, which is both faster (2.7s) and the only way they can be overridden at all.
Tests: 6200 backend tests pass, up 26. Mutation-verified: ingesting legacy documents
here, silently falling back when unprovisioned, marking complete without confirming
retrievability, and collapsing the two timestamps are each caught by the specific
test that exists for them.
* feat(kb): tombstone deletion sagas and the report-only reconciler
Task group 10. This is the code that deletes billed AWS resources, so the design is
built around two questions: can it leak, and can it delete the wrong thing.
Deletion saga (tombstones.py)
The tombstone is written BEFORE the AWS call and cleared ONLY after AWS confirms
absence. That ordering is the entire mechanism — a crash mid-delete leaves a work
item rather than a silent leak. Absence is established by polling
ListKnowledgeBases until the name disappears, never by the delete call returning:
deletion is async and took 2-6 minutes when measured, so the window tolerates 8.
Tombstones carry NO TTL; letting TTL reap them would recreate exactly the
silent-leak class this design closes. The service role cannot be deleted while any
knowledge base still references it, and DELETE_UNSUCCESSFUL raises its own
exception, emits a metric and annotates the tombstone rather than being swallowed as
a completed delete.
Reconciler (reconciler.py)
Joins paginated, tag-filtered ListKnowledgeBases against KB_Records. Two branches
matter more than the rest:
* AWS-only is an orphan, and is deletable only if AWS's OWN reported createdAt is
more than 24 hours old. Age-gating on when the reconciler first noticed it would
mean a reconciler that had been down for a week either waits another day on
everything or, inverted, deletes every in-flight create. A missing or unparseable
createdAt fails closed.
* Record-only NEVER deletes the record. A record with no AWS knowledge base means
the vectors are gone, not the documents: the source bytes are still in S3 and the
DOC# rows are still valid, so the corpus is rebuildable and the record is the only
pointer to it. It is marked vectorState: missing and left alone. delete_item
appears zero times in the module.
It ships report-only: arming is a separate flag, an empty string reads as off, and
the per-run action limit is clamped to a ceiling the environment cannot lift.
The audit caught one genuinely dangerous defect, worth recording because it would
have been invisible in review. lambda_handler forwarded an `armed` field from the
invocation event into reconcile(), so an EventBridge target carrying a constant
{"armed": true} — or any principal holding lambda:InvokeFunction — would have
deleted user resources while every piece of reviewable configuration still said
report-only, leaving only an Invoke in CloudTrail. The environment flag is now the
only arming path, and the regression test is parametrised over the boolean case that
the previous, misleadingly-named test never covered.
Tests: 6301 backend tests pass, up 101. Network isolation is proven rather than
asserted: the suites were run under a plugin that raises on any non-loopback socket
connection, and the plugin was itself verified by pointing a throwaway test at a real
AWS endpoint and watching it trip. Mutations all caught by the correct test,
including age-gating on discovery time, deleting the record on record-only, clearing
the tombstone before confirmation, report-only performing a delete, an empty-string
flag reading as armed, and a TTL added to a tombstone.
Requirements 14.1's schedule and 14.7's deployed mode need EventBridge wiring and
deploy config, which belong to the platform group under the rule that backend code
never deploys before the IAM and resources it requires.
* docs(kb): handoff document and accurate task-list state
Groups 1-10 were complete but only group 10 was ticked. Ticks 1-9 and their
subtasks, leaving 26 subtasks across groups 11-15.
Adds HANDOFF.md, written for a session with no memory of this work. It records the
constraints that were expensive to discover and would otherwise be rediscovered the
hard way:
* The DynamoDB one-GSI-per-UpdateTable limit, and that this feature has consumed the
rag-assistants budget for whatever release ships it. Violating it took production
down in 1.12.0.
* That DynamoDB cannot do arithmetic inside a ConditionExpression, which is why the
byte cap uses a single accumulator compared against a client-computed literal.
* That `total` and `ttl` are reserved keywords. Both bit this feature.
* Why kb_backend exists as its own package with an empty __init__, and that
apis.shared.embeddings is separately fine to import.
* That a module constant bound as a default argument cannot be patched, which cost a
33-second test that silently ignored its own override.
* The four distinct ways a mutation test can report a false pass, all four of which
happened here: an anchor that never matched, orphaned expression values turning a
ValidationException into a happy-path failure, a syntax error mistaken for a
detection, and simply not checking which test failed.
* The ten defects already found and fixed, so none is reintroduced — including the
reconciler arming bypass and the reserved-namespace metric grant that would have
deployed cleanly and published nothing.
Also records what is deliberately not built: the feature is unreachable by design
until group 14 registers the managed backend, and both the real Docker image and the
reconciler's EventBridge wiring are correctly deferred to the platform group.
* feat(kb): app-side authorization, IAM-enforced sharing, publication semantics
Group 11 of the managed-kb-migration spec (Requirement 25 / evaluation gate
§14.3). Managed KB ships two features whose names overstate what they provide,
so neither is trusted as the authority: AWS's own multi-tenant guidance calls
metadata filtering "filter-level (logical) isolation, not IAM-enforced", and
states that ACL-aware retrieval "is not authorization" — its identity is email
only, with no alias resolution, and a mismatch fails silently. On an OIDC
claim-mapped platform that is a worse primitive than an explicit app-side check.
`kb_access` resolves the invoking user's grant from the existing assistant
permission model, and the facade refuses to contact any backend without one.
The `access` parameter is required and keyword-only rather than defaulted:
forgetting it is then a TypeError at the call site, while a genuine denial
passes None and fails closed. Both production callers already resolved the
permission a few lines earlier, so the answer is handed over rather than
re-derived — the inference route now keeps the permission it was discarding.
A `KbAccess` cannot be constructed with a permission outside the read set, so
holding one is evidence the model was consulted; holding a string is not.
The ordering is the requirement, so the tests assert on a recording backend's
call list, not on the returned value: an empty list is also what an authorized
user with an empty corpus gets, and a check that runs after retrieval is an
audit log rather than an access control.
`resource_policy` narrows a shared corpus to named infrastructure identities.
The module says plainly what it is not — every user retrieves through the same
runtime role, so no resource policy can distinguish user A from user B; what it
buys is that a multi-user corpus stops being readable by anything in the account
holding the wildcard grant this platform's own identity policy uses. Staleness
is stored state, not an event: a policy attaches to the KB ARN, so any cycle
producing a new awsKbId silently drops sharing, and an event hook is only as
complete as the list of places that fire it (already two, with rehydration a
known third). The record stores the identifier the policy was applied to and
`policy_is_stale` compares; a path that forgets to re-apply is repaired by the
next call instead of regressing silently.
`kb_publication` keys reclaim exemption on `is_on_shelf`, not `is_listed`. An
admin requesting changes on a live listing leaves it serving but moves its state
out of LISTED_STATES, so by state name alone a reclaim pass would delete the
corpus behind an agent users can still see in the store.
Two defects found and fixed while testing:
- `is_reclaim_exempt` treated an empty mapping as an unreadable record, which
would have exempted every unheld knowledge base and made the predicate
vacuous. `None` (absent) and `{}` (read, no holds) are now distinct.
- The IAM surface had no resource-policy grant at all, so Req 25.6 would have
deployed as inert code. Added as its own statement on its own role: the writer
of a sharing policy must not also be able to create, delete, ingest into or
retrieve from a knowledge base, and a test asserts no retrieval identity ever
receives it. `GetDocumentContent` appears in the policies this caller writes
but not in its own grant — writing a permission is not holding it.
Nothing is deployed and nothing new is reachable: the managed backend is still
unregistered, so this changes behaviour only by adding the access gate to the
existing legacy path.
Verified: 6,358 backend tests pass (was 6,301; same 5 pre-existing Strands SDK
contract failures), 614 infra tests pass (was 611), ruff clean on every file
touched. All 12 new guards mutation-tested — each was broken in turn and the
specific named test that covers it failed, with the anchor match count, the
mutant's parseability and the identity of the failing test all checked so a
mutation could not report a false catch.
* feat(kb): opt-in dual-read pilot that legacy always wins
Group 12 (Requirement 18). An opted-in knowledge base has both backends answer
the same query; legacy is served and the managed result is kept only as an
observation, so the rollout can rest on evidence from our corpus rather than on
a three-document benchmark.
Latency is the constraint that shapes the design. Managed Retrieve measured a
662-695 ms p50 against legacy's 257 ms, so anything that awaited both would
nearly triple the retrieval leg of every piloted turn -- while returning correct
results and passing every other assertion. So the facade starts the managed read
*before* awaiting legacy and detaches the comparison afterwards: a piloted turn
does exactly the waiting an unpiloted one does. The test that holds this makes
the managed backend sleep three seconds and asserts the facade still returns
promptly, which is the only formulation that fails when the calls are gathered.
Legacy is served even when it is empty. An empty legacy result is a finding, and
reaching for the other engine's answer would destroy the measurement and change
what users see in the same move.
Details worth the reasoning:
- The opt-in reads `is True`, not truthiness. A truthy string left by a
hand-edited record must not enrol a knowledge base into paying for a second
retrieval every turn -- the same shape as the reconciler-arming defect, where
a permissive read of a flag turned a report-only job into a deleting one.
- The detached tasks are held in a module-level set with a done-callback.
create_task returns the only strong reference; drop it and comparisons
silently stop being recorded under load, which looks like "the pilot found
nothing interesting".
- A legacy failure cancels the managed task the facade started. Left alone it
would pay for a Retrieve nobody reads and surface as an unretrieved task
exception.
- Overlap is Jaccard, because it is symmetric: a ratio against one side's length
reads as agreement when one backend simply returned fewer documents, which is
the case most likely to occur while the managed corpus is catching up.
- A document is ranked by its best chunk, not its last, or the correlation
measures chunking rather than agreement.
- A correlation over a single shared document is None, not 1.0, which would make
a pilot on small corpora look like perfect agreement.
- `emit_value` is separate from `emit_count` so the unit is a call-site
decision. A latency published as Count is not merely mislabelled: CloudWatch
graphs and alarms on it as a rate, invisibly, until someone reads the board.
- The facade now reads the KB_Record once and passes it to the resolver, so the
pilot flag costs no extra DynamoDB round trip. `load_record` collapses "absent"
and "unreadable" to `{}` because this feature gives both the same answer.
Still unreachable in production: the managed backend is unregistered, so
`start_managed_read` returns None for every record regardless of the flag. That
makes setting the attribute in the database harmless before task 14.
Verified: 6,386 backend tests pass (was 6,358; same 5 pre-existing Strands SDK
failures), 614 infra tests pass, ruff clean on every file touched. 10 new guards
mutation-tested individually -- including the gathered-instead-of-detached
mutation, the truthy-flag mutation and the last-chunk-ranking mutation, each
caught by its own named test.
* docs(kb): handoff reflects groups 11-12 and two new defects
* feat(kb): migration dispatcher and the shadow/verify/promote/retain worker
Group 13 (Requirements 15, 16, 17). One EventBridge tick reads the sparse
KbWorkIndex, takes at most 20 records, and async-invokes the worker once each.
The worker takes a lease, executes exactly ONE step, and returns.
One step per invocation, because a 20-document text corpus is ~3 minutes but a
20-PDF corpus can exceed an hour -- per-document parse time was measured at
37-264 s and dominates everything. A worker that ran the whole machine would
sometimes finish and sometimes hit its timeout, and a timeout mid-shadow is
indistinguishable from a crash. Stepping means every interruption lands on a
recorded state with a conditional guard in front of it.
Nothing is mutated in place: the live corpus keeps serving from legacy through
shadow and verify, the managed copy is built alongside, and promote is one
conditional write. That is also why rollback moves no data -- the legacy index
was never touched, so returning to it is an attribute REMOVE.
Convergence rather than dual-write. The upload path stays authoritative; the
worker snapshots the doc-id set, migrates it, then runs catch-up passes until a
pass finds nothing new. Each DOC# record is re-read immediately before that
document is ingested, not once per batch: a PDF batch takes minutes and the
deletion this guards against is most likely to land inside exactly that window.
THREE DEFECTS THE PROPERTY TEST FOUND, each fixed in the code rather than
weakened in the test:
1. A resumed migration re-ingested the entire corpus. The completed-document set
lived inside `migrationProgress`, which a later write replaces wholesale, so a
crash near the end of 25 documents re-parsed all 25. Now a separate
`migratedDocIds` string set, updated with ADD per batch -- additive, so a
crash loses only the batch in flight, and immune to two workers clobbering
each other.
2. `promote_engine` permitted a SECOND promotion. Every guard it had -- state,
generation, converged progress -- stays true after a successful promotion, so
a crash between the promotion and the state transition promoted again with a
fresh promotedAt over the real cutover moment. Worse, two genuinely concurrent
workers would BOTH succeed, which is what Requirement 15.10 forbids. Now
guarded on `attribute_not_exists(retrievalEngine)`; rollback REMOVEs the
attribute so a deliberate re-promotion still works.
3. Fixing (2) then made a resume after a successful promotion mark the migration
`failed` -- a promoted knowledge base with no retention window. `run_promote`
now treats "already promoted" as success and continues to retain, and
re-reads before deciding so a genuine guard failure still raises.
The property is also stated honestly. "Each document ingested at most once" is
not achievable: a worker can die between a successful Ingest and the DynamoDB
write recording it, and no transaction spans Bedrock and DynamoDB. What is
asserted instead is that each document appears in the corpus exactly once
(because customDocumentIdentifier makes a re-ingest a replace) and that redundant
re-ingests are bounded by one batch -- which is the assertion that caught (1).
Verification is a manifest of document_id + content identity, never count parity:
count parity is satisfied by a corpus with the right NUMBER of wrong documents,
which is exactly what a migration racing an upload and a delete produces. Plus a
canary retrieval built from the corpus's own filenames, because a knowledge base
can hold documents while returning nothing and a constant query like "test" can
legitimately match nothing in a real corpus.
Verified: 6,457 backend tests pass (was 6,386; same 5 pre-existing Strands SDK
failures), 614 infra pass, ruff clean. 24 guards mutation-tested. Five of those
mutations initially survived and each was a real finding: a limit assertion the
final trim masked, a derivation whose test was vacuous because the priority list
happened to be complete, a `match=` pattern loose enough that the wrong check
satisfied it, and an `except LeaseLost: raise` that was dead code because the
lease was taken outside the try. All four were fixed, not annotated.
* docs(kb): handoff reflects group 13 and four more defects
* docs(kb): restore defect-list ordering in the handoff
* feat(kb): register the managed backend, fleet metrics, and tagged teardown
Group 14's backend and infrastructure half (14.0, 14.1, 14.2, 14.6, 14.7). The
frontend half -- 14.3 upgrade UX, 14.4 failed-document surfacing, 14.5 admin
surface -- is untouched.
SPEC GAP: nothing registered the managed backend. `register_backend` was defined
in task 4.2 and called by nothing; task 8.3's note that it would do the
registering was never carried out and no other task picked it up. Every one of
the 15 groups could have been completed with the feature unreachable, because a
promoted record raises BackendUnavailable -- a correct fail-safe and a useless
signal, seen only by the single user whose knowledge base was migrated.
Both adapters are now installed in the registry at import, not by a startup call
somebody has to remember. That is free because their module bodies are
stdlib-only with lazy clients, which the boundary test now asserts for
`managed_backend` as well -- the day someone hoists a `boto3.client(...)` to
module scope, every Lambda image carrying any part of the package pays for it.
Registration does not make the feature live: nothing resolves to managed until a
record says so, and only a promotion writes that.
It also exposed a test passing for the wrong reason.
`test_no_managed_call_when_no_managed_backend_is_registered` assumed no backend
existed; once one did, it kept passing because a real adapter was starting and
failing against no AWS. It now creates the condition it names.
METRICS (14.1). KbCount / KbStorageGB / KbIdleGB via EMF, once per reconciler
pass, into the same `{prefix}/ManagedKb` namespace the PutMetricData grant is
conditioned on. Idleness is max(own lastRetrievedAt, bound agents' lastUsedAt),
never retrieval alone: an agent can be invoked all day and retrieve nothing,
because retrieval only fires when the query matches -- so a corpus judged by
retrieval looks most abandoned exactly when its agent is busiest with questions
its documents do not answer, and the follow-up spec's eviction pass would delete
it. A knowledge base with no signal at all is reported as *unmeasured*, not idle;
counting it as idle would make every freshly provisioned corpus look abandoned.
The lastRetrievedAt write is throttled by a conditional freshness floor and
detached from the request, and is guarded on `attribute_exists(SK)` so a metrics
side effect cannot create KB_Records across the 1,692 legacy rows.
Also implemented Requirement 20.13's metrics READ grant, which existed only as a
comment -- so the reconciler could not have read Bedrock's own Invocations.
COST ATTRIBUTION (14.2). docs/specs/managed-kb-cost-attribution.md. Keying on
`AmazonBedrock` returns $0.00 successfully, which is worse than an error; keying
on service code alone blends KB into the Runtime-memory line that is 73% of the
AgentCore bill. Filter on usagetype.
TEARDOWN (14.6, 14.7). Managed knowledge bases are runtime-created, so
delete-stack does not touch them: left behind they keep billing at $5.00/GB-month
while being invisible in the CloudFormation console. scripts/teardown/managed-kb.sh
deletes only tag-matched ones, before any stack, because the Bedrock service role
lives in PlatformStack and Bedrock needs it to perform the delete.
THREE DEFECTS IN THAT SCRIPT, all found by running it rather than reading it:
1. Infinite spin. The wait loop advanced a counter by the poll interval and
stopped at the timeout; at an interval of 0 the counter never advanced, so an
`aws` call and a `python3` parse ran per iteration at full CPU. A test set the
interval to 0 to be fast and ran for SIXTEEN HOURS. Interval clamped to >=1,
non-numeric values fall back rather than erroring under `set -e`, and the loop
is now bounded by an attempt budget so termination does not depend on
arithmetic a later edit could break.
2. Timing-dependent false absence. `list | cut | grep -Fxq` -- grep exits on its
first match and closes the pipe, so under `pipefail` the upstream's SIGPIPE
(141) became the pipeline's status and the check answered "absent" about a
knowledge base it had just found. Whether it happened depended on how much
output was already written. Now a capture then a match, failing safe.
3. Fail-open discovery. A failed `list-knowledge-bases` was swallowed: `set -e` is
suspended for a function called in a condition, so the loop continued, the
parse of an empty response failed silently, and the run reported a clean
teardown having deleted nothing. Every step now carries an explicit
`|| return 1`, and the discovery walk refuses to continue rather than reading
an unreadable account as an empty one.
Verified: 6,511 backend tests (was 6,457; same 5 pre-existing Strands SDK
failures), 616 infra (was 614), ruff clean on every file touched, `bash -n` clean.
24 guards mutation-tested. Four of those mutations initially survived and each was
a real finding: two mutations aimed at lines the named test never reached, one
test that asserted only termination where the guard controlled the *value*, and
one that checked for the presence of strings a mutation preserved. The harness
itself was also wrong twice -- an on-disk check that reported false failures for
additive mutations, whose replacements contain their own anchors.
* docs(kb): handoff reflects group 14 backend half and three more defects
* fix(kb): one source of truth for the managed KB tag contract
Four components in three languages have to agree on the tags that identify this
platform's knowledge bases, because a tag-filtered ListKnowledgeBases is how the
reconciler and teardown both scope themselves. Nothing asserted the agreement, and
they had drifted three ways:
- `provisioning.build_tags` wrote keys `prefix`/`env` from PROJECT_PREFIX and
ENVIRONMENT -- neither of which the provisioning Lambda is given, so every
knowledge base would have been tagged with hardcoded defaults regardless of
project or environment.
- `tombstones.project_tag_filter` was a hand-written MIRROR of that function,
documented as one. A mirror is a second implementation, and the only thing
keeping two implementations equal is that nobody has edited one yet.
- `kb-migration-construct.ts` declared entirely different key names
(ManagedKbPrefix, ...) and exported them plus the correct values as environment
variables that NOTHING READ.
- `scripts/teardown/managed-kb.sh` read a third pair (CDK_PROJECT_PREFIX,
CDK_ENVIRONMENT) and matched on `prefix`/`env`.
The failure was silent in the worst way. Writer and reconciler agreed with each
other because both fell back to the same defaults, so knowledge bases would have
been created, found and reconciled normally. Only teardown disagreed, and its
symptom was matching nothing and reporting success -- Requirement 20.8 failing
into a leak of resources billing at $5.00/GB-month with no CloudFormation console
to notice them in. Two environments in one account would also both have claimed
`agentcore/dev` and treated each other's corpora as their own.
The construct was right; the Python was wrong and nothing bridged them. So the
keys now live in `kb_backend/tags.py` as constants mirroring the construct's, the
value resolution is one function with one fallback chain used by writer and reader
alike, and `provisioning.build_tags` / `tombstones.project_tag_filter` are thin
delegations rather than parallel implementations. The knowledge base's AWS *name*
resolves through the same helper, so a name saying `prod` while the tag says `dev`
is not reachable.
Keys are namespaced (`ManagedKbPrefix`, not `prefix`) because generic keys collide:
many accounts carry an organisation-wide cost-allocation tag literally called
`env`, and if something else writes it our filter compares against a value we did
not set -- a teardown that skips a knowledge base it owns.
One production consequence beyond the rename: `reconciler._delete_orphan` read the
`appKbId` AWS tag by its old name to anchor an orphan's tombstone, so it would have
anchored on the AWS id instead. The KB_Record *attribute* is separately named
`appKbId` and deliberately stays that way.
Test fixtures now build tags through the canonical helper instead of spelling them
out. A fixture that hardcodes tag keys keeps passing after the keys change under
it, which is precisely how this drift stayed invisible.
Verified: 6,530 backend tests (was 6,511; same 5 pre-existing Strands SDK
failures), 617 infra (was 616), ruff clean, `bash -n` clean. 17 cross-layer drifts
mutation-tested -- each component's idea of a key or a variable changed in turn,
and every one is caught by the new contract test. One survivor was a real gap: the
knowledge base name's prefix was untested despite the docstring claiming it could
not disagree with the tag.
* docs(kb): handoff records the tag contract fix
* feat(kb): owner-facing upgrade surface and the enrolment path it needed
Task 14.3, plus 14.4's surfacing half. Closes the last gap in the migration
control path: before this, nothing wrote a record into `shadow`, so all fifteen
task groups could have been finished with the feature unreachable.
The task was scoped as frontend-only, which is how the gap hid — the missing
piece was an HTTP surface nobody had planned. `apis/app_api/kb_upgrade/` is its
own package rather than living in `kb_migration/`, because that package's four
handlers share one size-constrained Lambda image and this one imports the
embeddings-pulling assistants package for the permission model.
Enrolment is two conditional writes, not one put. `KbRecord.to_item` does not
write the GSI7 work keys — only `set_migration_state` maintains them — so the
obvious single-put enrolment yields a record that reports an upgrade in progress
while being invisible to the dispatcher's sweep forever.
- Four endpoints under /assistants/{id}/knowledge-base/upgrade: read status,
start, retry, dismiss notice. Read allows any resolved permission and reports
canUpgrade=false to a viewer; the three writes require owner or editor.
- Derived phases (none|available|in_progress|succeeded|failed) so the client
never learns that shadow/verify/promote are all "working on it".
- Two public transitions on kb_backend/records.py: retry_from_failed (one atomic
write, guarded on generation AND still-failed) and dismiss_upgrade_notice.
- The offer is gated on MANAGED_KB_MIGRATION_ENABLED, the dispatcher's own flag,
so the offer and the capability cannot disagree.
- Stranded documents are surfaced before the user commits, splitting unsupported
format from processing failure against the ingestion pipeline's own extension
list. `deleting` is deliberately shown though the ordinary document list hides
it — 101 of the 200 affected production records sit there.
- app-api-environment.ts now passes MANAGED_KB_MIGRATION_ENABLED to the task
that reads it. It was absent, so the card would have rendered nothing in every
deployed environment regardless of the flag.
Deferred: Req 21.2's one-click document retry. Ingestion is S3-event-triggered
and no reprocess endpoint exists, so it needs new backend against a live
pipeline. The card directs the user to re-upload, which works today.
Tests: 69 backend, 41 frontend, 6 infra. Ten backend and four other mutations
verified against named tests; one survivor found an unrecognised migrationError
leaking verbatim into the card.
* fix(kb): pass the Environment tag value instead of guessing it (#885)
A development deploy tagged its managed knowledge bases
`ManagedKbEnvironment=prod`, while `scripts/teardown/managed-kb.sh` looks for
`dev`. Teardown would therefore have matched nothing and reported a clean run,
leaving billed Bedrock knowledge bases behind — the same symptom as the original
tag-contract drift, one layer up and invisible to the tests written for it.
Two fallbacks that never agreed:
* `managedKbEnvironmentTagValue` → `config.tags.Environment ?? (production ?
'prod' : 'nonprod')`. `config.tags` is `{ManagedBy: 'CDK'}` — no Environment
key — and `config.production` is `true` in every environment, because
platform.yml never passed a production flag and cdk.context.json says true.
* the teardown script → `MANAGED_KB_TAG_VALUE_ENVIRONMENT` → `ENVIRONMENT` →
`CDK_ENVIRONMENT` → `dev`.
Neither is wrong on its own, which is why nothing caught it: the existing
contract tests check that all three languages agree on the tag *keys*, and that
the Python writer and filter share one fallback chain. Nothing compared the value
the CDK computes against the value the shell defaults to.
Fixed by passing the value explicitly per environment rather than by making the
fallbacks agree — a fallback correct for dev is wrong for prod and vice versa, so
the only safe version is not to rely on one.
CDK_TAG_ENVIRONMENT: development=dev, production=prod (GitHub Environment vars)
Forwarded as the FLAT dotted context key, and read as one. `--context a.b=c` sets
`context['a.b']` and does not merge into a nested object, so a nested-only read
silently ignores the operator's own flag — the trap that already cost this repo
the managed-KB byte caps and then the alarm thresholds.
`config.production` is deliberately left alone. It also governs PITR on the
artifacts table, X-Ray sampling, alarm thresholds and notifications; flipping it
for dev is a defensible change but a much wider one than a tag fix, and it should
be its own decision.
Verified by synth: with the value passed the Lambdas carry `dev`; without it,
`prod`. Four new tests guard the plumbing end to end, since an omission anywhere
in the chain silently reinstates the guess.
* feat(kb): ship the real kb-migration Lambda image
Replaces the bootstrap stub with the actual handlers, which is the last thing
standing between an enrolled knowledge base and a completed migration. Until
now PlatformStack shipped four no-op Lambdas and the dispatcher ticked into
them every 15 minutes.
Seven artifacts, not the five the spec anticipated — `deploy-image-lambda-one.sh`
also needed per-function cases:
* backend/Dockerfile.kb-migration
* backend/src/apis/app_api/kb_migration/requirements.txt
* the kb-migration case in scripts/build/build-one.sh
* four cases in scripts/build/deploy-image-lambda-one.sh
* build-kb-migration + deploy-kb-migration-code in backend.yml
* entries in both hand-maintained supply-chain lists
⚠️ THE boto3 PIN IS THE FEATURE, NOT HYGIENE.
The Lambda base image at our pinned digest bundles boto3 1.40.4, whose packaged
bedrock-agent model offers `type` enum ['VECTOR', 'KENDRA', 'SQL'] and has no
`managedKnowledgeBaseConfiguration` shape at all. Measured, not assumed. Without
boto3==1.43.68 installed over it, every CreateKnowledgeBase call fails with a
ParamValidationError naming a parameter that looks correct in our source. Removing
or downgrading the pin breaks the feature silently at runtime and nothing else in
the repo would notice, so three tests now guard it — including one that asserts
the *capability* (MANAGED in the enum, the embedding pin members, FLOAT32
uppercase, all four document operations) rather than just the version string.
The COPY surface is five directives for a 16-module closure with boto3 as the
only dependency. That is the `kb_backend` boundary paying off: its `__init__` is
empty and its module scope is stdlib-only, so this image never pulls
`apis.shared.assistants` and the embeddings stack behind it. No FastAPI, no
pydantic — KB_Record is a dataclass for exactly this reason.
Verified in the built container on linux/arm64: all four handlers import, all
nine kb_backend modules import, boto3 resolves to 1.43.68, MANAGED is in the
enum and all four document operations are present. Also verified by staging the
Dockerfile's COPY list into a bare tree and asserting every closure module
resolves from inside it and nowhere else — module-level imports alone prove
little here, since most of these are function-local and only run on invocation.
Three mutations confirmed caught by named tests: pin removed, pin downgraded to
the base image's 1.40.4, and a COPY dropped.
Tests: 6,772 backend (5 pre-existing Strands SDK failures).
* fix(kb): give the dispatcher the worker name it actually reads
No knowledge base could migrate. The first real dispatcher tick after the image
landed raised, and would have raised every 15 minutes forever:
RuntimeError: KB_MIGRATION_WORKER_FUNCTION_NAME is not set
The construct set `MANAGED_KB_WORKER_FUNCTION_NAME`; `dispatcher.py` reads
`KB_MIGRATION_WORKER_FUNCTION_NAME` — the house convention its siblings use
(`KB_SYNC_WORKER_FUNCTION_NAME`, `SCHEDULED_RUNS_WORKER_FUNCTION_NAME`). The IAM
grant was already correct, so only the name was wrong. kb-sync does not have this
bug for one reason: `kb-sync.test.ts` asserts the variable is present.
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`. Requirement 15.11's
configured retention window was therefore being silently replaced by the code's
30-day floor. Safe, because the fl…
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reconciles
developwithmainafter the 1.19.0 squash-merge (#991,f2068cf7).The whole point is to make
maina genuine ancestor ofdevelop. Squashing recreates the divergence and the conflicts come back next release. (Feature branches intodevelopsquash as usual — the backmerge is the deliberate exception.)What's in it
Release artifacts only — the auto-merge was clean, no conflicts, because
develophadn't touched these files since the last reconciliation andmainonly added on top:VERSION→ 1.19.0CHANGELOG.md/RELEASE_NOTES.md— the 1.19.0 entriesREADME.mdbadge + current-release lineNo feature code moves in this direction —
developalready had all of it.Verified before committing
🤖 Generated with Claude Code