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

[mrsabath]

Phase / scope

Phase	Phase 3 — per-user isolation, declarative agents, triggers (DESIGN_PHASE3.md)
What this changes	§9 rollout steps 1–4 plus T5: tenancy keys, TopicSet, the user registry, signed userkey/depth/agent, per-user stores, the ownership index, and AgentSpec.
Phase 3 tasks NOT here	T6 (ensure_subscribed), T7 (owner-scoped reads), T8 (transcript auth), T9–T12, T14–T21. EB_TENANCY_MODE=multi does not work end to end until T6 — no response is consumed.
Phases it does not touch	Phase 0/1's wire contract and KEDA model are unchanged, and single mode reproduces Phase 2 byte for byte — the 525 pre-existing test functions pin that. It extends Phase 2's identity work (#879) rather than replacing it.
Relationship to the other drafts	#884 (Phase 2 signing, deployable) — no file overlap; both add EB_/ER_ variables, so whichever merges second re-runs test_manifests.py's classification check. #890 (Phase 2 docs) — both edit agentdocs/README.md; whichever merges second resolves one table row and one reading-order entry. #890 deliberately leaves the DESIGN_PHASE3.md row to this PR.

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, agent — 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. Every
test that existed before this branch passes untouched — 573 collected at the merge-base
1098094, all still passing here; three test fakes were widened to match a producer
signature, but no assertion was weakened or removed. (The "656" previously quoted here
was never a real count of anything — it is the figure retracted in Testing below.) 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

• 875 passed, 5 skipped at aeefb19: +245 unique test functions (525 at the
  merge-base 1098094 to 770), 880 collected with parametrisation. ruff check and
  ruff format --check clean.
  
  Skip count varies by host: 5 needs_kubectl always, plus
  test_graceful_shutdown.py where a sandbox denies subprocess spawn — so 5, 6 or 7
  skips are all consistent runs of the same tree.
  
  Two earlier versions of this line were wrong. The first said "656 baseline + 139
  new", which diffed collected counts across a moving baseline; the second
  (840/+217/845) was accurate for 6ebdd80 and went stale across two more rounds of
  fixes. IMPLEMENTATION_REPORT3.md is the figure of record — a PR body cannot stay
  right about a branch that is still moving.
• 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.

[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.

[aslom]

Re-review at c25a0c77, after 6ebdd80, 3278d75 and c25a0c7. All five must-fixes
from round 1 are genuinely fixed — I re-read each against this head rather than taking
the comment thread on trust:

Round-1 must-fix	State at c25a0c77
Index never seeded (single-mode regression)	seed_from() + __main__.py:142 backfill ✔
create_group dropped userkey	handlers.py:333 passes it; row and members colocate ✔
close/cancel unauthenticated cross-tenant mutation	_authorize_mutation, 404 per §6.2 ✔
agent outside SIGNED_ATTRS	signing.py:219, one canonicalisation break ✔
userkey → path join unvalidated	tenancy.is_valid_userkey, invalid ⇒ shared/ + unattributed ✔

The four deferred suggestions landed too, and tests/test_topic_routing_e2e.py is the
right shape — faking at KafkaProducer so the real handler, Producer and TopicSet
all run, asserting the topic string. It earned its keep immediately by finding the
ValueError-swallowed _publish_started.

IMPLEMENTATION_REPORT3.md is a good report, and I checked its numbers rather than
reading them. They hold up (see Verification below).

What I found this round

Two must-fixes, same root cause, both reproduced against c25a0c77. 6ebdd80
introduced validation of the request's agent name — correctly, at the HTTP boundary —
but placed it after the writes in both submit paths. So a 400 now leaves committed
state behind:

• create_group has already created the group row and published group.started.
  With an Idempotency-Key, every retry then returns 200 {created: false, members: []}
  — the group never gains a member, never completes, never notifies. That is the exact
  failure the userkey= comment at handlers.py:327 describes, reached by a route the
  fix did not close.
• start_agent has already written the session and the prompt row and claimed the
  correlation id, with nothing published — a transcript showing a turn that was never
  submitted, which is §4.7's own description of the /continue 500.

The fix is to move both _agent_for calls above the first write. Worth noting
tests/test_review_fixes.py:347 already asserts exactly the right thing for /continue
(get_prompts(corr) == []); the new agent-name test 25 lines below it asserts only the
status and publish_request.assert_not_called().

On the design-vs-code gap (the other half of the ask)

I walked §2–§9 against the tree. The deltas are recorded honestly and §3 of the report
is accurate as far as it goes; three things are not in it:

1. __main__.py's backfill is per-start, not one-time — it opens every tenant store
   and re-reads all_correlations on every boot. That is the N-store startup seeding
   §2.6 rejects, and §6.1a's "seeded once" plus the report's "once, on the first start
   after an upgrade" both describe something the code does not do. Measured below.
2. AgentSpec.limits is parsed, validated by nothing, and enforced nowhere. Skill
   says plainly that it carries sha256 for T19; Limits makes no such disclaimer, so
   timeout_s = 900 in an agent.toml loads clean and does nothing.
3. §8.3 was not updated with §2.6. It still says the signed set "changes twice" and
   names only userkey and depth, while §2.6, signing.py and ce.py now all say
   three — and signing.py:216 cites §8.3 as the authority for the rule it broke once
   instead of twice.

Everything else I checked lines up: EB_NTFY_TOPIC_SECRET_PATH's "Required in
multi" and EB_SUBSCRIBE_TIMEOUT_S are unenforced, but the new status header covers
them under T6/T11 explicitly, and §6.5's deletion API (forget_tenant,
correlations_for, is_tombstoned, userkeys) shipping without callers is T21
territory and reads as deliberate.

Verification I ran locally

Check	Result
python -m pytest in eventing/ at c25a0c77	853 passed, 7 skipped (860 collected)
same at base 10980948	549 passed, 7 skipped (556 collected)
ruff check . / ruff format --check . @ 0.11.4	clean; 135 files already formatted
Report's "525 → 757 unique test functions (+232)"	confirmed exactly (grep -c '^def test_' over tests/)
Report's "860 collected"	confirmed
CI	11/11 green

I also applied all six code suggestions below to a scratch worktree and re-ran
everything: 853 passed, 7 skipped, ruff check and ruff format --check still
clean, and each one closes the behaviour it was written for. After the two reorderings
the create_group reproduction above becomes:

POST /v0/groups {"prompts":["a","b"],"agent":"../../etc"}  Idempotency-Key: retry-me
  -> 400 Bad Request
  topics published: []
  groups in alice's store: []
POST again, same key, with "agent":"triager"
  -> 202 Accepted {"created":true,"members":["even-firefly-4773","hopeful-heron-9632"]}

My 7 skips vs the report's 5: test_graceful_shutdown.py:88,101 skip here because this
sandbox denies subprocess spawn, so 855 passed, 5 skipped is consistent with a host
where they run. The 5 needs_kubectl skips match.

The PR body's Testing section is now stale — it still reports 840 passed,
+217 unique (525 → 742), 845 collected from 6ebdd80, and its T4 row omits agent;
the "656 tests that existed before this branch" line above it is the figure you already
retracted. IMPLEMENTATION_REPORT3.md has the right numbers.

REQUEST_CHANGES for the two orphan-write paths only. Nothing in the tenancy core moved
in the wrong direction this round — the structural fix (test_topic_routing_e2e.py) is
the most valuable thing in the diff, and the ordering bug it did not catch is a
_agent_for placement, not a tenancy defect.

[mrsabath]

Round three is in 258da7a — both orphan-write paths and all three design/code gaps. Pushed before this comment, so your review at c25a0c77 predates it.

Both must-fixes were my own round-1 fix introducing a new bug class. 6ebdd80 added agent-name validation at the HTTP boundary — right place, right reason — and put it after the writes. I reproduced each against c25a0c77 before touching anything:

POST /v0/groups {"prompts":["a","b"],"agent":"../../etc"}  Idempotency-Key: retry-me
  -> 400,  topics: ['kev1-st-alice-c8ff0431-responses'],  groups: ['wild-weasel-1995']
  retry, same key, valid name -> 200 {created: false, members: []}

Permanent, because the row is keyed on the Idempotency-Key — and it reads as success. Exactly the failure the userkey= comment I added in the same round describes, reached by a route that fix did not cover. start_agent was the quieter one: session row, prompt row and a claimed correlation id, nothing published, and nothing ever un-claims the id since tombstoning is for deletion rather than abandonment.

Both _agent_for calls now run before the first write, with a comment at each site saying why the ordering is load-bearing. Your observation about tests/test_review_fixes.py:347 was the useful part — the /continue test already asserted get_prompts(corr) == [] while the agent test 25 lines below asserted only status and publish_request.assert_not_called(). Asserting the absence of published output and the absence of committed state are different checks, and I had written the weaker one directly beneath the stronger one.

§4.8 of the report records that fixing one instance of an orphan write did not teach me to look for the class — the same lesson §4.2 records about the eviction contract, learned a second time.

The three gaps are all real and all fixed:

• Per-start backfill. You were right that it is not one-time: it opened every tenant store and re-read all_correlations on every boot, which is the N-store startup seeding §2.6 rejects. Both §6.1a and the report described something the code did not do. Now gated per scope on a persisted seed_state row, checked before the store is opened — so an already-seeded tenant is never opened and never scanned, which is the cost §2.6 actually objects to. seed_from was always INSERT OR IGNORE, so repetition was harmless to correctness; what repeated was two SQLite connections and an indexed scan per tenant before the socket binds. Measured at ~210 ms per restart for 100 tenants × 200 correlations, and §2.6's own worked example (100 × 10,000) extrapolates to ~6 s added to every boot. Skipping is sound because nothing can add an unindexed id to a seeded store — every mint claims as it mints.
• AgentSpec.limits was parsed, validated by nothing, enforced nowhere. Skill says plainly that its sha256 is for T19; Limits made no such disclaimer, so timeout_s = 900 loaded clean and did nothing. It says so now, per field. Its conversions also escaped the module's SpecError contract — timeout_s = "900s" raised a bare ValueError past run_agent's handler instead of becoming the phase=error event §5.1 promises.
• §8.3 vs §2.6. Fixed, and the embarrassing part is that signing.py cited §8.3 as the authority for a rule §8.3 stated over the wrong count.

Two more from the same pass, both found by looking for the class rather than the instance: a tombstoned correlation could be re-claimed by the NULL owner (row[0] == userkey reads as "already mine" when both are NULL) — unreachable from Minter.mint, which checks exists() first, but reachable the moment a correlationid arrives from outside, which §7's triggers will bring. And registry.parse accepted agent unchecked, the only field it did not validate, in a function whose docstring promises every inconsistency is a refusal.

On the stale PR body — you were right, and it was stale in three places, not one. Now corrected: 859 passed / 6 skipped at 258da7a, +237 unique (525 → 762), 865 collected; the T4 row names all three signed attributes. I also removed the live "656" claim under The additive guarantee, which I had retracted in the Testing section while leaving it asserting upstairs — the real figure is 573 collected at the merge-base, and I measured it rather than deriving it. I have added a line saying IMPLEMENTATION_REPORT3.md is the figure of record, because a PR body cannot stay right about a branch that is still moving, and this is the third time I have corrected these numbers.

On your 7 skips vs my 6: test_graceful_shutdown.py spawns a real EventBridge, so it skips wherever subprocess spawn is denied and runs elsewhere. 5 (needs_kubectl only), 6 and 7 are all consistent runs of the same tree; the body now says so rather than quoting one host's number as the truth.

Dzięki — the reproduction you included is what made the Idempotency-Key interaction obvious, and the test_review_fixes.py:347 comparison is the kind of finding I would not have got to by re-reading my own diff. (Thanks.)

[aslom]

Verified at 258da7a. All eleven round-2 findings are resolved, and I checked each
against the tree rather than against the replies — including mutation-reverting every fix
to see which ones the suite actually holds.

#	Finding	Fix	Verified by
1	create_group 400 after the group row + group.started	_agent_for moved above groups.create	repro: nothing published, no row, retry returns 202 with both members
2	start_agent 400 after session/prompt/claim	moved above mint_for	repro: no topic, no session, index count 0
3	backfill per-start, not one-time	seed_state(scope, done_utc) + needs_seeding gate	measured 210 ms / 101 opens → 1.1 ms / 0 opens per restart
4	[limits] raised past SpecError	conversions wrapped	all three fields now raise SpecError
5	limits parsed, enforced nowhere, undisclosed	Skill-style disclaimer	docstring says "not yet enforced", names which task owns each field
6	tombstone re-claimable by the NULL owner	explicit deleted_utc check	Collision("… is tombstoned …") for both NULL and a tenant
7	registry.parse accepted agent unchecked	validate_name at parse	startup refusal names the bad label; blank still falls through
8	the missing "a refusal writes nothing" assertion	added, plus a create_group test with the retry	mutation: 6 and 1 failures respectively
9	§8.3 stale on the signed set	"changes once, for three attributes" + agent + a note on what the earlier revision said	read
10	README's orphaned "Two of these…"	Phase 3 block moved below, own #### Phase 3 heading	read; also fixed "Two notes" introducing three, and added the T6 warning where an operator will read it
11	report §1 counting its own commit	"As of 3278d75" with both figure sets	read

Three of these went further than asked, correctly. §3 of the fix for #3 skips the Store
construction as well as the scan, which is where the cost actually was — so the restart
penalty is gone rather than reduced. #2 moved above mint_for rather than merely above
the writes, which is the version that does not burn an id. And #10 surfaced two more
things the original insertion had broken.

IMPLEMENTATION_REPORT3.md §4.7 and §4.8 record this round, and §4.8 opens by admitting
that §3's conformance claim was asserted rather than checked. A report filing its own
over-claim as a finding is the right instinct.

Verification I ran

Check	Result
python -m pytest in eventing/ @ 258da7a	858 passed, 7 skipped (865 collected)
ruff check . / ruff format --check . @ 0.11.4	clean, 135 files formatted
Independent repro of all 7 code findings	11/11 assert the fixed behaviour
Startup backfill, 100 tenants × 200 correlations	1.1 ms, 0 store opens on restart (was 210 ms / 101)
CI	11/11 green
Commits	12, all Signed-off-by, Assisted-By not Co-Authored-By
.claude/ / .vscode/ changes	none

One thing worth doing before this grows further

Three of the six code fixes in 258da7a have no regression test anywhere. Reverting
agentspec's try/except, claim's tombstone check, or registry's validate_name
leaves all 858 tests passing. The three that do have tests are solidly held — 6, 1
and 4 failures under mutation. The commit adds exactly 5 test functions (757 → 762),
which accounts for the four seed_state tests and the one create_group test and
nothing else.

That is not a correctness problem — all six fixes are present and I verified each
behaviourally. It is that the standard this PR has applied since 6ebdd80 ("every fix
has a regression test, verified by reverting its fix") was applied to half of this
commit, and the three without one are the three whose absence is invisible.

Non-blocking, and noted rather than commented on: the limits fix took the try/except
without the timeout_s < 0 or max_events < 1 range check, so timeout_s = -5 still
loads clean. Defensible now that Limits is documented inert — flagging only so the
decision is on the record for whoever wires up the Popen wrapper.

Verdict

Approving. No blockers remain: both must-fixes are closed and reproduced closed, the
six suggestions are implemented, the three documentation gaps are corrected, and nothing
in the tenancy core moved in the wrong direction across three rounds. CI is green, the
author is a maintainer, there are no agent/IDE config changes, and EB_TENANCY_MODE
still defaults to single, which the pre-existing 525 test functions continue to pin.

Two things this approval is not, both of which the PR now states itself rather than
leaving me to say:

• It is not approval to switch on multi. T6 means no response is consumed, T7/T8 mean
  reads and PUT /transcript are as open as Phase 2's, and §3.6 is clear that topics
  without ACLs are organisation rather than isolation. The README, the design header and
  the report all say so in those words now.
• It is still a draft, so the merge call is yours. If you would rather land the test
  coverage for those three fixes here than in the T6 branch, that is the only thing I
  would hold it for — and I would not hold it long.

[aslom]

[suggestion] These four are the right tests and they hold — but three of the six code
fixes in this commit have none, and the suite does not notice when they are gone.

I mutation-reverted each fix in 258da7a independently and ran the whole suite, not
just this file:

Fix reverted	Tests that fail
start_agent's _agent_for back after the writes	6
create_group's _agent_for back after groups.create	1
seed_from no longer marks the scope seeded	4
agentspec.parse's [limits] try/except removed	0 — 858 passed
OwnerIndex.claim's deleted_utc check removed	0 — 858 passed
registry.parse's validate_name(agent) removed	0 — 858 passed

The arithmetic agrees: this commit adds exactly 5 test functions (757 → 762), which
is the four above plus test_a_bad_agent_name_on_create_group_writes_nothing. Nothing
was written for the other three.

Not a correctness finding — I verified all six behaviourally and they work. It is that
the standard this PR set in 6ebdd80 ("every fix has a regression test, and I verified
each test is load-bearing by reverting its fix one at a time") was applied to half of
this commit, and the half without tests is the half whose absence cannot be seen. Two of
the three are also the ones whose reasoning is about a future caller — claim's
tombstone check is explicitly closed "where the invariant is written down" rather than
where it is currently reachable, and a guard with no test is exactly the guard a later
refactor drops while staying green.

Three short ones, in the same file, would close it:

def test_bad_limits_raise_SpecError_not_ValueError():
    """`run_agent` catches only `SpecError`, so a bare ValueError from these three
    conversions escapes the `phase=error` path §5.1 promises."""
    for field, value in (("timeout_s", '"900s"'),
                         ("max_output_bytes", '"4MB"'),
                         ("max_events", '"lots"')):
        with pytest.raises(agentspec.SpecError):
            agentspec.parse(f"[limits]\n{field} = {value}\n", name="triager")


def test_a_tombstoned_id_is_never_reclaimed(tmp_path):
    """`forget` clears `userkey` to NULL, so `claim(corr, None)` read as "already mine"
    and succeeded — contradicting the guarantee `forget` makes."""
    idx = OwnerIndex(tmp_path / "eventbridge")
    idx.claim("happy-otter-0001", None)
    idx.forget("happy-otter-0001")
    for owner in (None, UK):
        with pytest.raises(Collision, match="tombstoned"):
            idx.claim("happy-otter-0001", owner)
    idx.claim("lucky-finch-0002", None)      # a live id is still idempotent
    idx.claim("lucky-finch-0002", None)


def test_the_registry_refuses_a_bad_agent_name():
    """The only field `parse` accepted unchecked. Unvalidated, it reaches `ce_agent` and
    the runner rejects it — an async `phase=error` on every request from that user."""
    bad = json.dumps({"version": 1, "users": [
        {"issuer": "github", "userid": "mrsabath", "agent": "Not A Label"}]})
    with pytest.raises(registry.RegistryError, match="DNS-1123"):
        registry.parse(bad)
    ok = json.dumps({"version": 1, "users": [
        {"issuer": "github", "userid": "mrsabath", "agent": "triager"}]})
    assert registry.parse(ok).users[0].agent == "triager"

Two import notes, since I hit both: agentspec is not imported in this file at all, so
the first one needs from eventrunner import agentspec (locally or at the top).
Collision is not in the owner_index import line either, but the file already imports
it locally once — around test_a_duplicate_mint_is_refused — so the local form matches
what is there. The blank-agent case is worth a line in the third as well: agent: ""
must still fall through to ER_AGENT_NAME rather than being refused as an invalid label,
which the if agent: guard gets right and a future tightening could break.

I ran all three against 258da7a and against each fix reverted individually: each passes
at head and each fails against exactly its own revert, nothing else.

[aslom]

[nit] Stale by exactly the five tests this commit added — the same shape as §1, which
you fixed by naming a vantage point.

At 258da7a:

858 passed, 7 skipped   (865 collected)        <- 2 extra skips are my sandbox's
762 unique test functions (525 -> 762, +237)

against §2's 855 passed, 5 skipped, 860 collected and 525 → 757 (+232). The 5-test
delta is precisely test_a_seeded_scope_is_not_seeded_again,
test_seed_scopes_are_tracked_independently, test_an_empty_store_is_still_marked_seeded,
test_the_seed_mark_survives_a_restart and
test_a_bad_agent_name_on_create_group_writes_nothing.

§1's fix is the one to copy: name the commit the figures are from. A test-results block in
a document whose contract is "measured figures rather than estimates" will go stale on
every commit that adds a test, and dating it is cheaper than re-measuring it each time.

Worth keeping §2's own observation while you are there — the claude --help skip that
fires in CI but not locally is still true, and still the most useful line in the section.