feat(eventing): Phase 3 steps 1-4 + T5 — tenancy keys, per-user stores, declarative agents

[mrsabath]

Implements agentdocs/DESIGN_PHASE3.md §9 rollout steps 1–4, plus T5 — the
foundation for per-user isolation. Extends the Phase 2 identity work (#879) rather than
replacing any of it.

The design is a 12-step, 22-task phase, far larger than Phase 0/1/2 were individually.
§9 singles out the first four steps as the part that is safe to merge while multi has
never been switched on anywhere: "the first four steps are refactors with tests, and
everything risky is behind a flag that defaults off." T5 is the keystone that everything
from step 5 on depends on, so it is here too.

What landed

Task	What
T1	shared/tenancy.py — userkey(), TopicSet, ntfy_topic()
T2	TopicSet threaded through the producer, responses consumer and both mirrors
T3	Caller, the user registry, ce_userkey stamped on requests
T4	SIGNED_ATTRS += userkey, depth — one change, per §8.3
T5	Per-user stores, the global correlationid → userkey index, Minter uniqueness
T13	eventrunner/agentspec.py + build_cmd as a function of the spec
T22	The Secret-vs-ConfigMap rule pinned in test_manifests.py

Not implemented, deliberately: T6 (ensure_subscribed), T7 (owner-scoped reads), T8
(transcript auth), T9–T12 (capability keys, ntfy isolation, k8s_tenant.py), T14–T19
(triggers, fetched skills), T20 (Kafka ACLs), T21 (deletion).

The additive guarantee

EB_TENANCY_MODE=single is the default and reproduces Phase 2 byte for byte. The 656
tests that existed before this branch pass untouched — three test fakes were widened
to match a producer signature, but no assertion was weakened or removed. That is the
check §9 step 2 asks for, and it is why the largest diff in the phase is also the least
risky.

The default AgentSpec is held to the same standard:
test_default_spec_reproduces_phase2_argv_exactly compares against a literal
transcription of the pre-Phase-3 build_cmd, not a call into the current
implementation. Single-tenant mode also keeps its SQLite files exactly where Phase 2 left
them — directly in the bridge root, not under shared/ — so an upgraded deployment's
sessions and transcripts keep showing up instead of appearing to vanish.

Decisions worth a reviewer's eye

• The digest in userkey is the security control, not decoration. _slug is lossy so
  keys stay readable in kubectl get kafkatopics; that is safe only because 32 bits of
  SHA-256 over the canonical form are appended. a.b@x.com, a_b@x.com and a-b@x.com
  all slug to a-b-x-com and must not merge. The issuer is inside the hashed bytes too —
  otherwise a second configured issuer impersonates the first.
• Separate store FILES, not one table with a userkey column (§6.1). store.py has
  28 methods and every one touches tenant data; a WHERE userkey = ? remembered 28 times
  is one that will eventually be forgotten. Separate files make the mistake structurally
  unavailable — the connection is the tenant.
• The ownership index is UNIQUE on correlationid alone (§2.6). (userkey, correlationid) is right inside a tenant's store and exactly wrong here: two tenants
  minting one id produce two distinct tuples, never conflict, and the collision passes
  silently. That constraint is also what lets sessionuuid stay unsalted.
• An explicit topic list, not subscribe(pattern=…) (§3.2). A pattern is picked up by
  metadata refresh (5 min default), so a new user's first response can be published before
  anyone is subscribed — and with auto_offset_reset=latest it is never read, with no
  error anywhere.
• A request cannot widen the tool policy. max_turns/model still come from the
  request (Phase 0 wire parameters); permission mode, tool lists and system prompt are
  spec-only.

Bugs found in self-review and fixed

Each was confirmed by executing the real code, and each has a regression test that fails
without the fix:

1. /continue returned a WSGI 500 where the owning tenant could not be resolved —
   reachable via a deregistered user, a NULL submitter, or a mirror-backfilled
   correlation. Now a 503 (or 404) before anything is written.
2. Eviction closed stores that callers still held. Any multi-call read path
   (get_html makes five calls) could have its store closed by a concurrent request for
   another tenant → ProgrammingError: Cannot operate on a closed database. Eviction now
   drops the reference and lets CPython close at the last holder. The first attempt at
   this fix only pinned the SSE path — which closed the easiest case to see and left the
   class open; the bug was in the eviction contract, not one caller.
3. GroupMirror never learned about tenancy — restart-orphaned groups were settled
   against the shared store (so never settled at all, with silence as the symptom), and
   replayed pre-Phase-3 events, which carry no ce_userkey, put member rows in shared/
   while the group row sat in the tenant's store. That is the normal upgrade path.
4. except Exception hid two things: a signature mismatch that silently stopped every
   group event from publishing, and a broken ownership index reported as "correlation ID
   space exhausted".

Two limitations stated rather than implied

1. multi mode is not yet isolation anyone should present as such. Per-user stores
   exist and reads route to the owning tenant's store, but nothing checks that the
   caller is that owner — T7/T8 are not in, so reads, /continue and PUT /transcript
   are as open as Phase 2's. §10 is also blunt that even once those land, separately-named
   topics without Kafka ACLs are organisation, not isolation.
2. GET /v0/groups now requires an authenticated caller in multi mode, because the
   list must be scoped to someone. §6.2's route table does not mention it; flagging
   because it is a route that was open in Phase 2 and is the HTML page's own poller.
   multi already refuses anonymous callers at resolve_caller, so the practical blast
   radius is small — but say so if you would rather it waited for T7.

Testing

• 840 passed, 5 skipped. Counted properly this time: +217 unique test functions
  (525 on upstream/main to 742), 845 collected with parametrisation. An earlier version
  of this body said "656 baseline + 139 new", which was wrong — it diffed collected
  counts across a moving baseline. ruff check and ruff format --check clean.
• Both services verified to start in every tenancy configuration; all startup refusals
  print readable messages rather than tracebacks; the on-disk layout verified
  upgrade-safe.
• Test strength checked by mutation, not assumed. One gap found and closed: deleting the
  LRU's pinning check passed every test, because they asserted object identity rather than
  the notification path that is the rule's actual purpose. Removing it now fails 4 tests.

Review round 1 (#883, @aslom) is addressed in 6ebdd80: five must-fixes, including a
single-mode regression where the ownership index was never seeded from an existing store
(so exists() reported live correlation ids as free and a mint could reissue one), an
unauthenticated cross-tenant group mutation, agent missing from SIGNED_ATTRS while it
selects the tool-policy sandbox, and a userkey reaching a filesystem path join
unvalidated from an inbound Kafka header. Each reproduced before fixing; each has a
regression test verified by reverting its fix. Six design-doc contradictions corrected in
the same commit. Deferred, by agreement: the T22 subset check,
RequestsMirror/NtfyPublisher stores=, a handler-to-topic integration test, the
forget() tombstone decision, and the four phase documents.

Draft because this is the first slice of a 12-step phase — review comments here should
shape T6/T7/T8 before they are written.

Design: eventing/agentdocs/DESIGN_PHASE3.md (#881). Reading order for review: §9 for
why this scope, §2.3 for why the key ends in a hash, §6.1 for the per-user store trade,
§2.6 for the uniqueness guarantee.

🤖 Generated with Claude Code

[aslom]

Summary

Careful, well-reasoned work — 37 files, +4618/−153, implementing §9 steps 1–4 plus T5,
T13 and T22, with four fix(...) commits for defects found during the work and docstrings
that argue for their decisions rather than restating them. The two modules the design is
most specific about are faithful to it: userkey() puts the issuer inside the hashed bytes
with a \x1f separator for the right reason, and owner_index.py declares
correlationid TEXT PRIMARY KEY with a comment on why it must not be
(userkey, correlationid) — the #881 correction landing exactly as specified.

Requesting changes for five things, two of which I would not want merged in any form
and one of which affects the default configuration.

The single-mode regression is the serious one. __main__.py deletes the
Minter seeding loop and nothing backfills its replacement — OwnerIndex has no
seed-from-store path. On the first start after an upgrade the index is empty while
sessions.sqlite is full, so exists() reports ids as free that are in use. The space is
50 × 50 × 10,000 = 25M, so with ~1,000 existing correlations a mint has ~1-in-25,000 odds
of reissuing a live id, and the result is upsert_session overwriting a session and the
new prompt appending to somebody's old conversation. Phase 2's odds were zero. This
contradicts the headline guarantee, and no test covers the state that causes it because
the fixtures build index and store together.

Then two that make multi incorrect rather than merely incomplete. create_group
passes userkey to submit_members but not to groups.create(), so the group row lands
in the shared store while its members land in the tenant's — the batch reports 0 members
forever and never completes, and a retried POST returns members: []. And
close_group/cancel_group perform no authentication at all (_caller is called in only
three places in that file, neither of them these), then resolve any tenant's group from the
global index and mutate it. T7 covers reads staying open; it does not cover unauthenticated
cross-tenant mutation, which is new here.

Two more. agent is absent from SIGNED_ATTRS while ce_agent selects the AgentSpec
that supplies --permission-mode and the tool allowlists — so a forged attribute widens
the sandbox and the signature still verifies, which makes the signed configuration worse
than the unsigned one. And userkey reaches a filesystem path join unvalidated, from an
inbound Kafka header: .. escapes users/ into another tenant's store or out of the tree
entirely, verified by construction. tenancy.py is the only place that knows the key's
shape and exports no validator, which is the root of both.

Everything above was reproduced against 16158e50 rather than inferred. Six suggestions
and four nits follow, and two documentation comments.

On design conformance, which was asked for: the code is not fully reflected in
DESIGN_PHASE3.md. Six gaps, two of them contradictions — §6.1 still places
single-tenant stores in shared/ where the code uses the bridge root, and still asserts
"Closing is safe" about an LRU that commit 49aed05 proved unsafe. owner_index.py
appears in the document exactly once, in the status header this PR adds. Inline on that
header.

I have also set out what IMPLEMENTATION_REPORT3.md and README_PHASE3.md should contain,
and why the two Phase 2 documents are still owed — DESIGN_PHASE2.md §6 is a
test-results-and-findings report filed inside a design document, and its §7 "Running it?"
points at README_PHASE1.md for a phase that added the device flow, the approved-user
list, the break-glass path, a 300 s revocation window and the keyset rollout.

Author: mrsabath (MEMBER — maintainer, normal review posture)
Areas reviewed: Python (24 files), tests (12), Markdown (3). No YAML, Helm/K8s, Dockerfile, shell or CI in the diff.
Agent/IDE config (.claude/.vscode): none — §3.5a gate run for both +++ b/ and rename to forms, no match.
Secrets scan: clean. No CI workflow, dependency manifest, pyproject.toml, uv.lock or Dockerfile touched.
Commits: 8, every one Signed-off-by, DCO passing, all conventional prefixes.
CI status: all 11 checks passing — which is worth noting against the findings above: the suite cannot see the two multi defects, because every multi-mode group test calls GroupService.create directly with the userkey the handler fails to supply, and test_continue_tenancy.py mocks the producer so no topic string is ever asserted.

Checked and clear

• "No assertion was weakened" — true. test_auth.py and test_groups.py have one identical hunk each, widening a fake producer's publish_group_event signature and recording the new argument. Nothing removed or relaxed; test_manifests.py is purely additive.
• Hidden skips — none. The only skip is test_agentspec.py's disclosed skipif(shutil.which("claude") is None). Worth knowing that this is the test pinning §5.5's CLI flags, so it does not run in CI.
• Test strength generally — test_tenancy.py's collision corpus, test_owner_index.py's test_the_constraint_is_on_correlationid_alone and its concurrency test, test_store_registry.py's pinning and eviction regressions, and test_agentspec.py's literal Phase-2 argv transcription are all real and would fail on a regression. The two gaps are the ones noted inline.
• §8.3's coordination warning is narrower than written, in your favour. canonical() omits absent attributes and sorts by name, and nothing sets depth yet, so for every event a default deployment signs today the canonical form is byte-identical across this change. The two services do not have to move together unless multi is on.
• The GET /v0/groups question in the PR body is already answered by §6.2, which has that row — and also commits to filtering the list by owner, which this PR does not yet do.
• The test arithmetic needs reconciling: the body says "656 baseline + 139 new", but this PR adds 192 def test_ functions and removes none, and parametrisation only raises the collected count.

[aslom]

[must-fix] userkey reaches this path join with no validation, and in multi mode it
arrives from an inbound Kafka header (for_event → event.get(ce.EXT_USERKEY), line 141).
A userkey containing .. escapes users/:

'gh-alice-a1b2c3d4'            -> /data/eventbridge/users/gh-alice-a1b2c3d4
'../users/gh-victim-0bad0bad'  -> /data/eventbridge/users/gh-victim-0bad0bad   # another tenant
'../../../../tmp/pwned'        -> /tmp/pwned                                   # outside the tree
'..'                           -> /data/eventbridge                            # the bridge root

Store.__init__ does base.mkdir(parents=True, exist_ok=True), so the directory is
created, not rejected. A forged ce_userkey therefore files events directly into a
victim's responses.sqlite, or drops SQLite files anywhere the process can write.

Two reasons this is reachable today rather than theoretical:

• userkey is in SIGNED_ATTRS, but EB_REQUIRE_RESPONSE_SIGNATURE defaults to
  false, and in audit mode the event is stored unchanged. Verification is what would
  catch the forgery and it is off by default.
• §3.6 already concedes that in Tier A anything with network access to the shared broker
  can write to any topic. That is an accepted property for reading events; it must not
  also be a filesystem write primitive.

Note the invalid key is truthy, so it bypasses the unattributed branch above and
goes straight to the join.

The fix belongs in tenancy.py, which is "the ONLY place an identity becomes a name" and
currently exports no validator — so no consumer can check the shape the producer
guarantees:

# shared/tenancy.py
USERKEY_RE = re.compile(r"^[a-z]{2}-[a-z0-9][a-z0-9-]{0,23}-[0-9a-f]{8}$")

def is_valid_userkey(value: str | None) -> bool:
    return bool(value) and bool(USERKEY_RE.fullmatch(value))

Then treat an invalid key exactly like a missing one — shared/ plus unattributed —
so a forged value is counted and visible rather than acted on. Worth a round-trip test
asserting is_valid_userkey(userkey(iss, uid)) over the same adversarial corpus
test_tenancy.py already builds.

For what it is worth, agentspec.py gets this right 150 lines away: _resolve_under
refuses a path that "escapes the agent directory" and reasons explicitly about
../../../etc/shadow, and validate_name enforces a DNS-1123 label. The same gate is
missing on the one input that is genuinely attacker-supplied.

[aslom]

[suggestion] The uniqueness guarantee is enforced by the PRIMARY KEY, but the
error type callers depend on comes only from the in-process lock. If two writers ever
race past it, the bare INSERT raises sqlite3.IntegrityError, not Collision — and
Minter.mint catches Collision specifically, with a comment explaining (correctly) why
it must not catch Exception. So that path would surface a 500 rather than retrying.

Today the lock holds: one process, RLock, single-replica. It is load-bearing in a way
the docstring attributes to the constraint, though, and a second writer is exactly what
§6.5's deletion path and any future sweeper add. Letting the database be the authority
removes the dependency:

cur = self._c.execute(
    "INSERT INTO correlation_owner(correlationid,userkey,created_utc) "
    "VALUES (?,?,strftime('%Y-%m-%dT%H:%M:%SZ','now')) "
    "ON CONFLICT(correlationid) DO NOTHING", (correlationid, userkey))
if cur.rowcount == 0:
    row = self._c.execute("SELECT userkey FROM correlation_owner WHERE correlationid=?",
                          (correlationid,)).fetchone()
    if row[0] != userkey:
        raise Collision(...)

That keeps the idempotent-for-same-owner behaviour and makes Collision the only way a
conflict can present, lock or no lock.

[aslom]

[suggestion] created_utc is written at second granularity
(strftime('%Y-%m-%dT%H:%M:%SZ','now')), so ORDER BY created_utc DESC LIMIT ? is not a
deterministic "newest first" — a batch submitted inside one second has arbitrary order
among its rows, and with LIMIT smaller than that batch it returns an arbitrary subset.
GroupService.submit_members publishes a whole batch in a loop, so a 100-member group is
precisely the case that lands in one or two seconds.

Everything else on the event path uses millisecond precision (ce.now_iso() sets
timespec="milliseconds"), so this is also an inconsistency rather than only an
imprecision. Either match that, or add rowid as a tiebreaker (ORDER BY created_utc DESC, rowid DESC), which is free and total.

[aslom]

[nit] The docstring contradicts the code: "Note what this does NOT do: freeing the id
for reuse is deliberate" — but the DELETE does free the id, since exists() is the
Minter's only check and it will no longer see it.

Worth settling before T21 builds on this, because reuse has a consequence beyond tidiness:
sessionuuid = uuid5(NAMESPACE, correlationid) is unsalted by design (§2.6), so a reused
correlationid derives the same session uuid as the deleted one. A claude
transcript left on a runner's volume, or a checkpoint that outlived the delete, is then
resumable by the new correlation — a different user's conversation continuing into
somebody else's agent. Either keep a tombstone so ids are never reissued, or say
explicitly that deletion must also purge every transcript keyed on that uuid.

[aslom]

[nit] The docstring says static identities are "byte-exact: an operator typed them",
but the @ branch above this one catches them first — a static EB_AUTH_TOKENS name of
Ops@Example.COM canonicalises to Ops@example.com, not byte-exact. Harmless in
practice (and arguably the nicer behaviour), but the third bullet claims something the
code does not do, and this function's whole contract is that each branch is a deliberate
per-issuer claim. Either reorder on issuer == "static" or say the email rule applies to
any address-shaped identifier regardless of issuer.

[aslom]

[suggestion] This is the suite's multi-mode blind spot, and it is why the
create_group bug above survived 192 new test functions.

Every multi-mode group test calls GroupService.create(label=..., userkey=UK) directly
(also 113, 130), supplying by hand the one parameter Handlers.create_group fails to pass.
So the suite proves the tenanted path works when userkey is given, which is not the
property that matters — nothing drives Handlers.create_group in multi at all.

test_continue_tenancy.py:51 has the matching gap from the other side: producer = MagicMock(), and every "publishes to the owning tenant's topic" assertion checks
publish_request.call_args[1]["userkey"] — never a topic name. The file's stated hazard is
that "publishing to a shared topic would run the turn on another tenant's runner", and no
assertion anywhere covers it; test_topicset_wiring.py checks TopicSet and Producer
separately but never the handler → producer → topic chain.

One test per route that goes through Handlers with a real Producer against a fake Kafka
and asserts the topic string would have caught both the create_group defect and the
continue_agent 500. That is a better investment than more unit tests on the pieces, which
are already well covered.

[aslom]

[nit] str(named).strip() accepts any JSON value — a dict, a list, "../../etc" —
and ships it as ce_agent. agentspec.NAME_RE and validate_name already exist and
enforce a DNS-1123 label; the HTTP boundary is where that check belongs.

As it stands a bad agent name is answered 202 Accepted and surfaces minutes later as an
asynchronous phase=error event, which the submitter may never look at, instead of a
400 on the call that made the mistake.

[aslom]

[nit] Unguarded int(), so EB_MAX_OPEN_STORES=lots is a bare ValueError traceback
at startup while EB_TENANCY_MODE seven lines above raises a readable SystemExit. The
whole startup path is otherwise careful to print messages rather than tracebacks (the PR
body says so explicitly), so this is the odd one out.

Line 196 is the related gap: EB_TOPIC_PREFIX is unvalidated, so a prefix containing _,
. or a space produces exactly the illegal or JMX-colliding topic names that
tenancy.py's _slug docstring warns about — a topic a.b_c and a topic a_b.c collide
in Kafka's metric names and one silently overwrites the other's. The prefix is the one
component of those names _slug never sees.

[aslom]

[nit] payload.get("max_turns") or spec.max_turns replaced
payload.get("max_turns", 3), so a request with max_turns: 0 now silently gets the
spec's value instead of 0. The spec side rejects zero
(test_a_nonsensical_max_turns_is_refused), so the two paths disagree about the same
nonsensical input — one refuses it, the other rewrites it. is not None makes them agree,
or validate it at the HTTP boundary alongside the agent name above.

[aslom]

[nit] This assertion is flaky by construction. t.removeprefix("kev1-") is 26
characters of lower-cased base32, whose alphabet is a-z2-7 — so it contains g and h,
and the chance that "gh" appears somewhere in 26 characters is roughly
25 × (1/32)² ≈ 2.4%. It passes deterministically for this fixture's secret and
userkey, and will fail for about one in forty other combinations — including whatever
secret a deployment actually uses, if this ever runs against one.

It also adds nothing: assert "mrsabath" not in t on the line above is the property §4.1
cares about. The issuer prefix is two characters, so testing for its absence in a
130-bit digest is testing that a random string does not happen to contain a common bigram.
Dropping the line is the fix.

[mrsabath]

Thanks — this was a genuinely useful review. All five must-fixes are in 6ebdd80, and I
reproduced each one against 16158e50 before touching it rather than taking the report on
trust. Every fix has a regression test in the new tests/test_review_fixes.py, and I
verified each test is load-bearing by reverting its fix one at a time.

The single-mode regression was the one that mattered, and your reasoning about it was
exactly right. I confirmed it directly: a store with 50 pre-existing sessions plus a fresh
index reports 50 of 50 live ids as free. OwnerIndex.seed_from() plus a one-time
backfill from every store at startup closes it.

Worth recording why I walked into it, because the design contributed: §2.6 argues
convincingly that one SELECT per mint beats re-seeding a seen set on every start — and
in arguing that, it reads as though the index needs no seeding at all. Those are different
claims. §2.6 now distinguishes per-start seeding (correctly rejected) from one-time
backfill (required for correctness), and new §6.1a documents the index properly.

Finding	Fix
Index never seeded (single-mode regression)	seed_from() + startup backfill from every store
create_group dropped userkey	passes it; group row and members now colocate
close/cancel unauthenticated cross-tenant mutation	_authorize_mutation, 404 per §6.2
agent outside SIGNED_ATTRS	added alongside userkey/depth, one canonicalisation break
userkey → path join unvalidated	tenancy.is_valid_userkey(); invalid ⇒ shared/ + unattributed

On the path traversal — I took your suggestion as written, including treating an invalid
key exactly like a missing one so the forgery is counted and visible rather than
rejected. Your note that an invalid key is truthy and therefore bypassed the existing
not userkey branch was the part I would have missed; the validator lives in tenancy.py
for the reason you gave, and there is a round-trip test asserting
is_valid_userkey(userkey(iss, uid)) over the adversarial corpus.

On close/cancel: I agree "T7 is not in yet" does not cover it, and your framing is the
right one — reads staying open preserves a Phase 2 property, whereas unauthenticated
cross-tenant mutation is a capability this PR would have introduced. Also fixed the
three-routes-disagree problem you noted; all of them now keep the second return value.

Also addressed from your suggestions and nits: the known-but-unowned /continue 500 (I
reproduced the orphan prompt row — reading and publishing turn out to need different
answers, so _publishable_owner is now separate from _owner_userkey, keeping shared-tier
data readable); ON CONFLICT DO NOTHING so Collision is the only way a conflict
presents; millisecond created_utc with a rowid tiebreaker; agent name validated at
the HTTP boundary; max_turns: 0; EB_TOPIC_PREFIX and EB_MAX_OPEN_STORES validation;
the _canonicalise static/@ docstring.

You were right about the flaky test, including the arithmetic. "gh" not in 26
base32 characters fails about 2.4% of the time by construction. Dropped — the
"mrsabath" not in line above it is the property §4.1 actually cares about.

And right about my test count. It is +217 unique test functions (525 → 742; 845
collected with parametrisation), not the 139 I claimed. I had been diffing collected
counts across a changing baseline. Corrected in the PR body.

On design conformance

Fixed in-tree, under §9's "the code is the authority where implemented" rule:

• §6.1's layout — single-tenant stores are in the bridge root, not shared/, with the
  upgrade-safety reasoning that motivated it. I also fixed the same drift inside
  store_registry.py, whose module docstring reproduced the design's version while
  _dir_for did the opposite 40 lines below.
• "Closing is safe" — replaced with what 49aed05 established, including the detail
  you singled out: the first attempt pinned only the SSE path, which fixed the visible
  case and left the class open. The bug was in the eviction contract, not one caller.
• §6.1a — new, documenting owner_index.py: the correlationid-alone PRIMARY KEY,
  nullable userkey, Collision vs idempotent re-claim, owner_of's two-value return,
  the seeding requirement, and mint_for's honest degradation.
• §2.6 — "one new extension attribute" → four, with which three are signed and why.
• §3.1 — records the single-mode events/dead names.
• Status header — now says multi does not work end to end until T6, because no
  response is consumed. You are right that this is a more useful warning than "not yet
  isolated", which implies a working-but-unisolated mode.

Two corrections accepted: §6.2 does have the GET /v0/groups row, so requiring an
authenticated caller is the design's own position and needs no dispensation — and it also
commits to filtering the list by owner, which I have noted for T7. And §8.3's coordination
warning being narrower than written is a useful thing to know.

Deliberately not in this commit

Your remaining suggestions, which I would rather do as a reviewable second pass than bury
in this one: the T22 subset-direction check (you are right that it cannot currently fail
for the reason it exists), RequestsMirror/NtfyPublisher missing stores= — which do
look like the sweep in 77807b6 not reaching them — the handler→topic integration test,
and the forget() tombstone decision. That last one I want to think about rather than
answer quickly, since the unsalted sessionuuid makes id reuse a cross-user transcript
resume, which is a worse consequence than the docstring implies.

The four documents I have not started. Your argument that the findings are freshest now is
the right one and I would rather write IMPLEMENTATION_REPORT3.md while the eviction and
except Exception findings are still attributable to specific commits, so I will take it
next unless you would rather see the code suggestions land first.

All 11 checks passing on 6ebdd80.

[mrsabath]

Round two is in 3278d75 — the four remaining code items. All 11 checks passing, 855
tests.

T22 now checks the direction that enforces the rule

You were right that it could not fail for its stated reason. CONFIGMAP_VARS ⊆ found
catches a stale entry and nothing else; found ⊆ CONFIGMAP_VARS ∪ SECRET_PATH_VARS is the
direction that catches a new variable classified as neither.

Scoped to Phase 3's variables, with the 36 that pre-date §8.3 listed explicitly — and
test_the_baseline_list_is_still_accurate keeps that list from silently widening the
exemption, which was my worry about hardcoding it at all.

I took the deliberate-failure suggestion literally: injecting an unclassified
e("EB_UNCLASSIFIED_X") read into config.py turns the test red with the variable named.
The hardcoded ten-space indentation is now \s*, per the precedent you pointed at in the
same file.

RequestsMirror and NtfyPublisher

Both take stores= now. Your read looks right that these are where the 77807b6 sweep
stopped rather than a decision — Consumer and GroupMirror got it and these did not,
with no reasoning anywhere for the difference. The mirror's case was the worse of the two:
back-filling into the shared store means a correlation the bridge did not originate shows
a blank prompt on its owner's page, which is the exact gap the mirror exists to close.
ntfy's group-existence check needed the same treatment, which I would have missed if I had
only fixed the two functions you named.

The integration test was the right call, and it paid for itself twice

Faking at KafkaProducer rather than Producer means the real handler, the real
Producer and the real TopicSet all run, and the assertion is the topic string. I
confirmed it catches both defects it was written for by reverting each fix in turn.

Then it found a third thing nobody had flagged. Mutating create_group back to its buggy
form produced this in the captured output:

[groups] could not publish group.started for loud-weasel-0357:
    ValueError("userkey is required to name the 'responses' topic in multi mode")

_publish_started and maybe_complete were swallowing ValueError from TopicSet into
a log line — so a caller forgetting a userkey produced a group that silently never
announced itself. Exactly the shape of the TypeError I had already guarded there after
the first round, and I had not thought to widen it. Both guards now cover both, and a
genuine broker failure is still handled.

Your framing — "one test per route that goes through Handlers with a real Producer
against a fake Kafka, asserting the topic string, is a better investment than more unit
tests on the pieces" — is the most useful single suggestion in the review.

forget(): tombstone, never free the id

This is the one I said I wanted to think about rather than answer quickly, and the thought
changed the answer. I verified the mechanism instead of reasoning about it:

after forget, exists(): False
sessionuuid before     : 9189c66c-9eff-5cb5-92b2-0565ca420c33
sessionuuid if reissued : 9189c66c-9eff-5cb5-92b2-0565ca420c33
SAME session uuid: True

So your reading was right and the consequence is worse than the docstring implied.
exists() is the Minter's only uniqueness check, sessionuuid is unsalted by design,
and therefore a reissued correlationid derives the same session uuid — a claude
transcript left on a runner's volume, or a checkpoint that outlived the delete, becomes
resumable by the new correlation. One user's conversation continuing inside somebody else's
agent.

forget() and forget_tenant() now keep the row, set deleted_utc and clear userkey:
the id is reserved forever and the tombstone discloses nothing about whose it was, which
matters on a deletion path. I chose that over your other option — purging every transcript
keyed on that uuid — because that is a guarantee EventBridge cannot make: the volumes are
not all its own. Cost is one short row per deleted correlation. Migrated with ALTER TABLE, the same idiom store.py uses for prompts.submitter, with a test covering an
index created before the column existed.

And you were right about the docstring: it asserted "freeing the id for reuse is
deliberate" about a DELETE nobody had reasoned about. §6.1a now records the decision and
why, since the index is what enforces it.

---

Next: IMPLEMENTATION_REPORT3.md

Taking this next, and your argument for doing it now rather than later is the one I found
most persuasive in the review — the findings are attributable to specific commits today
and will not be in a month. Following Phase 1's report as the model: measured numbers,
failures with root causes, and an explicit list of what is not verified.

What I plan to put in it, so you can redirect me before I write rather than after:

• The findings, each with its commit and the symptom that hid it. The use-after-close
  eviction bug (49aed05) with the detail you singled out — first attempt pinned only the
  SSE path, fixed the visible case, left the class open, and the bug was in the eviction
  contract rather than one caller. The except Exception pair: a signature mismatch that
  silently stopped every group event from publishing, and a broken ownership index
  surfacing as "correlation ID space exhausted". Plus this round's ValueError variant,
  which is the same lesson a third time.
• Your five must-fixes as findings in their own right, including the one that matters
  most for a report: a single-mode regression shipped behind a headline claiming the
  opposite, and the suite could not see it because the fixtures supplied by hand the
  arguments the production call sites omitted.
• The deltas from the design as decisions, not drift — single-tenant store location
  chosen for upgrade safety, eviction by reference-drop, tombstoned deletion, the
  single-mode events/dead names.
• What is not verified, which for this phase is a long and more useful list than the
  verified one: no cluster run, no two-tenant deployment, multi not functioning end to
  end until T6, and the per-tenant throughput ceiling §10 asks for still unmeasured.

On the two owed Phase 2 documents — I agree they are owed, and your diagnosis of why the
gap shows is hard to argue with: §6 of DESIGN_PHASE2.md is a test-results-and-findings
report filed inside a design document, and §7 sends people to README_PHASE1.md for a
phase that added the device flow, the approved-user list, the break-glass path, a 300 s
revocation window and the keyset rollout. I would rather do those as a separate PR than
grow this one further, unless you would prefer them here.