Repository navigation
docs(eventing): Phase 2 — record the claims a docs review disproved, and how to check them - #890
Conversation
… check them The user-facing eventing pages (rossoctl/rossoctl#2609) restated every claim in DESIGN_PHASE2 for a reader who cannot see the code. Six review rounds then ran each one against eventing/ at d9677dd. Thirteen did not hold. DESIGN_PHASE2 §8 records them, rather than editing §§2.6, 3, 4.3 and 4.4 in place, so the reasoning stays readable as what was intended and §8 says what the code does. The four §4.4 promises that fail: audit mode logs nothing, a pinned group event is flagged and then applied anyway, a terminal signature stops proving who finished a run once INSERT OR REPLACE lets an unsigned frame overwrite it, and nothing checks that EB_SIGNING_KID is in the keyset. A fifth, promised nowhere and worse: EB_REQUIRE_RESPONSE_SIGNATURE with no keyset refuses nothing, silently, while the request side with no key refuses everything. CLAIMS.md turns that into five questions to ask of a control claim before it ships — which code path, what default, what happens half-configured, can an attacker choose the input the check reads, what does it become one release from now — plus how to run the check cheaply and what a correction should preserve. None of the thirteen was careless about mechanism; §4.4 is unusually precise. Each was written from the change that introduced the control and stayed true for the configuration that change was exercised in. Every one coexisted with a passing test suite. Tracked as #885, #886, #887 and #888; #889 came out of the same review without being a claim in any document. Docs only. No code changes, so this does not overlap #884, and it touches agentdocs/README.md where #883 also does — whichever merges second resolves one table row and one reading-order entry. Assisted-By: Claude Code Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
|
@mrsabath please merge CLAIMS into DESIGN_PHASE2.md or DESIGN_PHASE3.md as I think the claims are part of design and they should reflect the phase of the design they are related to |
@aslom on #890: "please merge CLAIMS into DESIGN_PHASE2.md or DESIGN_PHASE3.md as I think the claims are part of design and they should reflect the phase of the design they are related to." Taken. `CLAIMS.md` is gone and its content is now §8.5–§8.8 of `DESIGN_PHASE2.md`, directly after the thirteen findings it was derived from. Phase 2 is the right home: the findings are claims *this* document made, and §8.4 already handed off to the procedure, so the two halves were a section boundary apart for no reason. The content moved rather than being rewritten — 133 lines out, 135 in. Headings demote one level (the five questions become §8.5.1–§8.5.5) and the three references that assumed a separate file are re-pointed: "see its §8 for the list" is now "see above", and "`DESIGN_PHASE2.md` §8 is that shape" is now "§8.1–§8.3 above are that shape". **On the phase question, which is the substance of the request.** The findings are Phase 2's and the procedure is not, so §8 now opens by saying which is which: > §8.1–§8.4 are about **Phase 2** specifically: thirteen claims *this document* made > about the identity and signing controls, which do not hold in the code. §8.5–§8.8 are > the general procedure that came out of them, and apply to a claim made in any phase — > Phase 3's design makes claims of the same shape about tenancy and the signed attribute > set, and nothing here is Phase 2 only. That answers the question the PR body asked out loud ("is `CLAIMS.md` the right home? It is process guidance sitting among design and build records") rather than leaving the ambiguity in place. §8.5's own lead-in says the same thing for a reader who arrives there directly, and gives the reason it is filed here rather than at repo root: a procedure kept away from the findings that motivated it is a procedure nobody reads. `agentdocs/README.md`: the standalone index row is gone, the Phase 2 row now mentions both halves of §8, and the reading-order entry reads "**Writing a claim about a control, in any phase?**" pointing at §8.5 for the questions and §8.1–§8.3 for the findings — with "The findings are Phase 2's; the questions are not" stated explicitly, since that distinction is the whole point of where this now lives. Verified every `§N.N` reference in the merged document resolves to a heading that exists, and that no `CLAIMS` reference survives anywhere. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
aslom
left a comment
There was a problem hiding this comment.
Reviewed at 15b81da, treating §8 the way §8 asks to be treated: I checked every claim
it makes by running the code, not by reading it. All eleven enumerated findings hold.
One thing does not — the count.
Folding CLAIMS.md into §8 is the right resolution
15b81da does what I asked and the phase split is the part that makes it work. §8's
opening paragraph is the substance of the request: §8.1–§8.4 are Phase 2's findings,
§8.5–§8.8 are phase-agnostic, and it says which is which before a reader has to guess.
The README reading-order entry — "Writing a claim about a control, in any phase? …
The findings are Phase 2's; the questions are not" — closes it.
§§2.6, 3, 4.3, 4.4 is also the right attribution: §8.1 is §4.4, §8.2 is §3, §8.3 is
§2.6 and §4.3. I checked each.
Every finding verified against the code
eventing/ is byte-identical between d9677ddd and this PR's base 10980948
(git diff --stat is empty), so §8's "against eventing/ at d9677ddd" is still exactly
what a reader gets. Each item:
| § | Claim | Verified |
|---|---|---|
| 8.1 | "keyset alone verifies and logs" → logs nothing | ✔ response_decision returns (not require), so audit mode returns ok=True and kafka_in's if not ok: — the only print and the only self._rejected += 1 — never runs. Its own docstring says "hand back the reason so the caller can log it"; the caller does not. |
| 8.1 | group-event pinning flagged then applied | ✔ ran it: an approved runner-01 signing a group.completed gives ok=False, "expected a signature from kid 'eb-01', got 'runner-01'", and it still routes to on_group_event |
| 8.1 | INSERT OR REPLACE lets an unsigned frame overwrite the verified row |
✔ store.py:124, on (correlationid,sequence) |
| 8.1 | refuses without EB_SIGNING_KID, never checks kid ∈ keyset |
✔ __main__.py:68-74 raises; no membership check anywhere |
| 8.1 | REQUIRE=true + no keyset refuses nothing |
✔ ran it: ok=True why='not checked', forged terminal stored as the answer, banner says response verification OFF. And verify_event returns False with no key, so the request side refuses everything — the two sides do fail in opposite directions |
| 8.2 | GET /v0/groups unauthenticated, 100 most recent |
✔ no auth.resolve in list_groups; all_groups(limit=100) |
| 8.2 | /status lists every member id; PUT /transcript unauthenticated |
✔ auth.resolve appears at exactly two call sites, start_agent and create_group |
| 8.2 | ntfy suppression reads the event's own ce_groupid |
✔ ntfy.py:188 |
| 8.3 | §2.6 says "Both are unsigned"; they are in SIGNED_ATTRS |
✔ §2.6:189 vs signing.py's tuple |
| 8.3 | SPIRE: EdDSA only, and keyset.load rejects a JWKS |
✔ signing.py:344; fed it a real JWKS document — ValueError: key for kid 'keys' must be a string |
| 8.3 | flat keyset = attribution, not restriction | ✔ EB's own key must be in the keyset for the pinning to work, and select(kid) then verifies it on any run's answer |
Also checked: all five issues exist, are open, and their titles match what §8 attributes
to each — including #889 being cited in §8.7 as not one of the thirteen. §8.6's
reproduction recipe works verbatim: python3 -m venv .venv && .venv/bin/pip install -q -e .
then from shared import signing, keyset, ce imports cleanly. And the suite is green with
all of it present — 549 passed, 7 skipped — which is §8.6's own point.
The one claim that does not hold is the count
"Thirteen" appears six times in DESIGN_PHASE2.md and twice in README.md. §8.1–§8.3
enumerate eleven items: four table rows plus the fifth, three bullets in §8.2, three in
§8.3. Thirteen is reachable by splitting two compound bullets, but a reader who counts
gets 11 — and in a section whose thesis is that unchecked claims survive because nobody
counts, the one number that can be checked in ten seconds is the wrong one to leave
unreconciled. Inline comment with both ways to fix it.
Answering your question
Finding only — do not put the fix in §8. Three reasons, all from the document's own
reasoning:
- §8.7 already draws the line: "These documents are the record of what was intended."
A finding is a fact about a named commit and stays true; a proposed fix is a hypothesis
that review will change, and §8.8 is specifically about suggested wording being a
hypothesis. A fix written into the design record acquires authority it has not earned. - "What the code does" already carries the fix's shape. "
requireis never consulted
without a keyset" tells a maintainer what to change without pre-empting how — whether
that is consultingrequireoutside the branch or refusing at startup is exactly the
argument #888 should host. - The §8.1 table's third column is the issue, and that is the right indirection. A fix in
§8 would be the fourth place the same decision is written down, after the issue, the PR
and the code comment — and §8.4's diagnosis is that claims rot when they are restated
away from the thing they describe.
If you want the record self-contained, the cheaper version is one sentence per row saying
which direction the fix goes, not what it is.
On the other drafts
Two corrections to the overlap table, both in your favour and one against:
- "#883 fixes none of them" is right. I diffed
kafka_in.pyacross #883's head:
nothing in the verification block moves — noprint, no counter, no change to the
if self._keyset is not Noneguard. It threadstopics/storesand nothing else, so
§8 stays accurate on top of it, as you say. - The README collision is one hunk, not two.
git merge-treeagainst #883's head
gives exactly one conflict, covering the Phase 2 and Phase 3 table rows together;
the reading-order entries merge cleanly. Resolving it means keeping your Phase 2 row and
#883's Phase 3 row (which drops "Design only — not implemented"). Worth knowing because
#883 is now approved, so it is likely to merge first and the rebase will be yours.
Verification summary
| Check | Result |
|---|---|
| Claims verified against code | 11 of 11 hold |
| §8.6's documented recipe | works verbatim |
| Suite at this base | 549 passed, 7 skipped |
| Issues #885–#889 | all exist, open, titles match their attribution |
eventing/ at d9677ddd vs base |
identical |
| CI | 11/11 green |
| Commits | 2, both Signed-off-by, no Co-Authored-By |
| Scope | docs only — 2 files, no code, no manifests, no tests |
Approving. The findings are accurate, the phase attribution is right, and the
procedure in §8.5–§8.8 is the most reusable thing in this repository's documentation.
The three comments are a count, a missing issue link and one imprecise mechanism — none
of them blocking, and the first is the only one I would not merge without.
| **Scope.** §8.1–§8.4 are about **Phase 2** specifically: thirteen claims *this | ||
| document* made about the identity and signing controls, which do not hold in the code. | ||
| §8.5–§8.8 are the general procedure that came out of them, and apply to a claim made in | ||
| any phase — Phase 3's design makes claims of the same shape about tenancy and the signed | ||
| attribute set, and nothing here is Phase 2 only. | ||
|
|
||
| Writing the user-facing pages for this path meant restating every claim in this | ||
| document for a reader who cannot see the code. Six review rounds then checked each | ||
| restated claim **by running it**, against `eventing/` at `d9677ddd`. Thirteen did | ||
| not hold. |
There was a problem hiding this comment.
[suggestion] "Thirteen" is the one claim in §8 a reader can check in ten seconds, and
it does not reconcile. §8.1–§8.3 enumerate eleven items:
| § | Items |
|---|---|
| 8.1 table | 4 rows — and logs, group-event pinning, INSERT OR REPLACE, kid-not-in-keyset |
| 8.1 | + 1, "A fifth, promised nowhere" |
| 8.2 | 3 bullets — §3.1 capability URLs, §3.2 PUT /transcript, ntfy suppression |
| 8.3 | 3 bullets — §2.6 signed-after-all, §4.3 SPIRE, flat keyset |
| 11 |
Thirteen is reachable by splitting two compound bullets — §8.2's first covers both
GET /v0/groups and GET /v0/groups/{id}/status, and §8.3's SPIRE bullet covers both the
algorithm and the file format — but nothing in the text marks them as two, so a reader
counting gets 11 every time.
The number appears six times in this file (lines 450, 458, 524, 532, 606, 639) and twice
in agentdocs/README.md (the Phase 2 table row and the reading-order entry), so it is
load-bearing for the section's own argument: §8.4 says "all thirteen were found" in the
half-configured cases, and §8.6 says "every one of the thirteen coexisted with a passing
suite". Both are claims about a set whose size the list does not support.
This matters more here than it would anywhere else in the repository, because §8 exists to
argue that unchecked claims survive review. A count that does not survive counting is the
same failure in miniature — and it is the first thing a sceptical reader will test.
Two ways out. Split the two compound bullets so the list reads 13, which is honest if
that is what the review found:
| **Scope.** §8.1–§8.4 are about **Phase 2** specifically: thirteen claims *this | |
| document* made about the identity and signing controls, which do not hold in the code. | |
| §8.5–§8.8 are the general procedure that came out of them, and apply to a claim made in | |
| any phase — Phase 3's design makes claims of the same shape about tenancy and the signed | |
| attribute set, and nothing here is Phase 2 only. | |
| Writing the user-facing pages for this path meant restating every claim in this | |
| document for a reader who cannot see the code. Six review rounds then checked each | |
| restated claim **by running it**, against `eventing/` at `d9677ddd`. Thirteen did | |
| not hold. | |
| **Scope.** §8.1–§8.4 are about **Phase 2** specifically: thirteen claims *this | |
| document* made about the identity and signing controls, which do not hold in the code — | |
| eleven items below, two of which carry a second finding in the same bullet (§8.2's | |
| capability-URL entry covers both the group list and the member list; §8.3's SPIRE entry | |
| covers both the algorithm and the keyset format). | |
| §8.5–§8.8 are the general procedure that came out of them, and apply to a claim made in | |
| any phase — Phase 3's design makes claims of the same shape about tenancy and the signed | |
| attribute set, and nothing here is Phase 2 only. | |
| Writing the user-facing pages for this path meant restating every claim in this | |
| document for a reader who cannot see the code. Six review rounds then checked each | |
| restated claim **by running it**, against `eventing/` at `d9677ddd`. Thirteen did | |
| not hold. |
Or say eleven and change the eight occurrences, which is simpler and loses nothing —
the argument in §8.4 and §8.6 does not depend on the number being 13.
Either way worth doing in this PR rather than later: the count is quoted into
README.md, and #883 is about to touch the row next to it.
| - **Group-member push suppression is defeated by omitting an attribute.** `ntfy.py` | ||
| decides on the *event's own* `ce_groupid`, not on group membership. A forged | ||
| non-terminal frame without it is pushed, stored in the member's transcript and shown | ||
| on the group page; a forged terminal without it is pushed at priority 5 even with | ||
| `NTFY_GROUP_NOTIFY_ERRORS` off, and does not count the member as failed. |
There was a problem hiding this comment.
[suggestion] This is the only one of the eleven findings with no issue link, and §8.7
two screens down says the rule outright:
And file the issue. Most of the thirteen were code bugs rather than wording mistakes,
and they are tracked as #885, #886, #887 and #888.
The other two bullets in §8.2 both carry (#887). This one carries nothing, and it is
not obviously covered by any of the four: #887 is "unauthenticated PUT /transcript
rewrites what an agent resumes from, and correlation ids are listable", which is the
first two bullets and not this.
I verified the finding itself — ntfy.py:188 is groupid = event.get("groupid"), the
event's own attribute, with no membership lookup — so this is about the record, not the
claim. It is also the finding with the most concrete user-visible consequence of the three
in §8.2: a priority-5 push for a forged terminal with NTFY_GROUP_NOTIFY_ERRORS off, and
the member not counted as failed.
Either file it and link it, or say explicitly that it is untracked and why — "no issue:
the fix is subsumed by #885's enforcement work" would be a complete answer if that is the
case. An unlinked finding in the section that tells people to file the issue is the one
inconsistency a reader will notice fastest after the count.
| | §4.4 says | The code does | Issue | | ||
| |---|---|---| | ||
| | "A keyset alone verifies **and logs**" | Verifies and returns a verdict. Nothing is logged or counted, so the observable reject rate the two-flag rollout depends on does not exist. | [#886](https://github.com/rossoctl/examples/issues/886) | | ||
| | The verifier accepts "**only**" EventBridge's kid for group events, so an approved runner "cannot forge a `group.completed` and end a batch early" | Flags it, then applies it anyway. `kafka_in.py` rewrites `phase`/`data` but keeps `final` and `groupid`, so the rewritten event still reaches `on_group_event`. The group mirror replays group events unverified on restart. | [#885](https://github.com/rossoctl/examples/issues/885) | |
There was a problem hiding this comment.
[nit] The finding and its consequence are both right; the mechanism in the middle
names the wrong field and the wrong causal step.
I ran it. An approved runner-01 signing a group.completed against bridge_kid="eb-01":
verdict : ok=False why="expected a signature from kid 'eb-01', got 'runner-01'"
after rewrite: type=dev.rossoctl.agent.group.completed.v1
groupid='eager-weasel-0787' final=None
is_group_event(evt) -> True <- the routing key
OUTCOME : reaches on_group_event -> complete_group
Two corrections:
- A group event carries no
finalat all (final=Noneabove) —ce.new_eventis
called without it inpublish_group_event, andon_group_eventnever reads it.final
is what matters for the member-answer row in this table, two rows down, not for this
one. - The rewrite is not why it still reaches
on_group_event.kafka_in.pyroutes on
ce.is_group_event(evt)— computed on the original event, before and regardless of
the rewrite. Even a rewrite that stripped every attribute would still route there.
Keepinggroupidis whycomplete_groupthen finds the group to end; keepingtypeis
whyon_group_eventdispatches to the completion branch.
So the precise version is one clause different:
| | The verifier accepts "**only**" EventBridge's kid for group events, so an approved runner "cannot forge a `group.completed` and end a batch early" | Flags it, then applies it anyway. `kafka_in.py` rewrites `phase`/`data` but keeps `final` and `groupid`, so the rewritten event still reaches `on_group_event`. The group mirror replays group events unverified on restart. | [#885](https://github.com/rossoctl/examples/issues/885) | | |
| | The verifier accepts "**only**" EventBridge's kid for group events, so an approved runner "cannot forge a `group.completed` and end a batch early" | Flags it, then applies it anyway. `kafka_in.py` routes on `ce.is_group_event(evt)` — the *original* event, independently of the rewrite — and the rewrite replaces only `phase`/`data`, so `type` and `groupid` survive and `on_group_event` ends the batch. The group mirror replays group events unverified on restart. | [#885](https://github.com/rossoctl/examples/issues/885) | |
Flagging a one-clause imprecision because §8.5.1 is "Which code path, by name?" and
this row is the worked example a reader will copy the style from.
|
Done in You put your finger on something the PR description was uneasy about and asked out loud: The content moved rather than being rewritten (133 lines out, 135 in): the five questions On "they should reflect the phase of the design they are related to" — that is the
And §8.5's own lead-in gives the reason it is filed here rather than at repo root: a
Verified every On phase clarity across the three open PRs, since this came up here and the same
The one remaining open question from the original description is unchanged: should §8 |
…kind-signed Follow-up to rebasing onto `main` now that #884 and #890 have merged. Both predicted conflicts were in `agentdocs/README.md` and resolved as planned — `main`'s Phase 2 row (which gained §8 from #890) plus this branch's Phase 3 row. `test_manifests.py` auto-merged, keeping #884's `allowed_secrets` fix beside this branch's Phase 3 classification. That left two things the merge did not resolve by itself. **Extending `SECRET_PATH_VARS` to #884's seed paths needed the rule split, not widened.** #884's review predicted this tuple would be extended to `ER_/EB_SIGNING_KEY_PATH` and checked that doing so stays green. It does for the ConfigMap half and would NOT for the literal-value half, because those two variables hold a **mount point, not key material** — and #884 deliberately sets them as container `env` entries so the path sits beside the volume that supplies the seed, which is what `configmap.yaml` asks for. So the tuple is now two: - `SECRET_VALUE_VARS` — the three whose *value* is key material. No literal value anywhere, which is the original assertion. - `SEED_PATH_VARS` — #884's two. A literal value is correct; a ConfigMap key is not. Both still obey "never from `config.toml`", so that check covers all five now where it covered three. Treating a path naming a secret as if it were the secret would have forced #884 to either revert its own correct fix or carve itself out of the rule — the distinction is the thing worth encoding. **`kind-signed` was not in `OVERLAYS`.** #884 added the overlay and every assertion in this file silently skipped it — including the seed-path rule the PR exists to respect, the `replicas` invariants, and the no-credential checks. Added, which is also what makes the new assertion meaningful: with `kind-signed` absent, injecting a seed path back into `eventing-config` passed; with it present, that regression fails as it should. Verified both ways. Tests: 899 passing (875 here + #884's 24 picked up in the rebase), ruff clean, all four overlays build. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
…s, declarative agents (#883) * feat(eventing): Phase 3 step 1 — tenancy keys and declarative agent specs DESIGN_PHASE3.md T1 and T13, the two tasks with no dependencies. Nothing here changes behaviour: `single` tenancy is the default everywhere and the `default` AgentSpec reproduces Phase 2's argv byte for byte. shared/tenancy.py (T1) `userkey(issuer, userid)` is the only place an identity becomes a name. The slug is deliberately 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, so two identities that slug identically still differ. The issuer is inside the hashed bytes as well as the prefix — without it, configuring a second issuer is a way to impersonate an identity on the first. The digest input is joined with \x1f so the issuer/userid boundary is unambiguous. Canonicalisation is per-issuer and minimal: GitHub folds case (and must agree with `ghauth.is_allowed`, which a test pins), email lower-cases only the domain per RFC 5321, static is byte-exact. Erring toward two tenants for one human is wasteful; the other direction is a leak. `TopicSet` is the one place §3.1's layout is written down, and refuses to name a topic without a userkey in multi mode rather than falling back to a shared one. `ntfy_topic` is HMAC-derived so the name carries no identifier (§4.1). eventrunner/agentspec.py (T13) One TOML file per agent, carrying the tool policy that §7.4 depends on — which is why §9 puts this before triggers. Validation refuses rather than coerces: a policy that silently drops the clause it could not parse would hand a trigger-driven agent the tool the operator meant to remove. A missing non-default agent is an error event, never a silent fallback to `default`. `build_cmd` takes a spec. The request still wins on `max_turns`/`model` (Phase 0 wire parameters), but the tool policy, permission mode and system prompt are spec-only: a request that could widen the sandbox is a request that can escape it. Tests: 105 new, 656 total passing, no existing test modified. The argv-equality test compares against a literal transcription of Phase 2's `build_cmd` rather than a call into the current one, so it catches a regression instead of following one. The `claude --help` flag assertion covers only the flags Phase 3 adds. `--max-turns` is supported but undocumented in `--help` on 2.1.270, so asserting the pre-existing flags would fail against a working deployment. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * feat(eventing): Phase 3 steps 2-4 — TopicSet wiring, user registry, signed tenancy DESIGN_PHASE3.md T2, T3, T4 and T22. Still no behaviour change in the default configuration: `EB_TENANCY_MODE=single` is the default, and the 656 tests that existed before this branch all pass untouched. That is the property §9 asks steps 1-4 to preserve — refactors with tests, everything risky behind a flag that is off. T2 — TopicSet threaded through (§3.2) One set, built once in `__main__` and handed to the producer, the responses consumer, both mirrors and the config, so §3.1's layout appears exactly once. `publish_request` takes a `userkey` and resolves the topic from it; one producer can send to any topic, so there is no second producer and no connection per user. The explicit-topic-list option, not `subscribe(pattern=...)`. A pattern is picked up by metadata refresh (5 min by 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 — the page stays empty and no error appears anywhere. The blocking `ensure_subscribed` that closes the remaining race is T6. GroupMirror takes the full topic list directly: it is one-shot at startup, so there is no new-user race to lose, and a tenant whose topic is not provisioned yet is skipped with a log line rather than costing everyone their group history. T3 — the registry and Caller (§2.4, §2.5) `Caller` carries the tenancy key so no caller derives it twice. `submitter_iss` is deliberately not the same as `issuer`: a static identity needs an issuer for its key to separate from a GitHub login of the same name, but Phase 2 made an absent `ce_submitteriss` mean "an operator typed this", and promoting it would upgrade an assertion into a verified claim. The registry refuses rather than skips — the opposite of `auth.parse_tokens`, because a skipped user is one whose topics exist and whose events have nowhere to go. A recorded userkey that disagrees with this build's derivation is a startup refusal naming both values, and two identities colliding on one key is refused, which is what covers §2.3's 32-bit birthday bound. `multi` mode refuses anonymous with 401 and an unregistered user with 403. Both disagree with the single-tenant defaults on purpose; `single` is byte-identical to Phase 2. T4 — SIGNED_ATTRS += userkey, depth (§8.3) One change, both attributes, because adding one changes canonicalisation and a signer and verifier on different versions disagree about every signature. Both have to be covered rather than merely present: a mutable `userkey` lets anything with topic write access file events into another user's history, and a resettable `depth` is not a hop limit. T22 — the Secret-vs-ConfigMap rule, pinned in test_manifests.py The secret-path variables are declared ahead of the tasks that read them (T8-T16) so the guard is in place when they land. The classification test only asserts the ConfigMap side against live code, and says so, rather than looking stronger than it is. Known gap, stated rather than hidden: `/continue` resolves the owning tenant from the recorded submitter so a resume reaches the right runner, but does not yet check that the caller IS the owner — owner-scoped reads and `?k=` keys are §6.2/§4.4 (T7/T9, step 5). Until those land, `multi` mode's `/continue` is as open as Phase 2's. The lookup also cannot distinguish a GitHub `alice` from a static `alice`; `multi` mode refuses both anonymous and static submissions, so GitHub is the only issuer that reaches it today, and this moves to T5's global index when that exists. Tests: 66 new (722 total), ruff clean. Verified both services start in every tenancy configuration and that all four refusals print readable messages rather than tracebacks. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * docs(eventing): record which of Phase 3 is implemented and which is still design The Phase 3 design doc said "nothing in this document is implemented yet" and the agentdocs index said "Design only — not implemented". Both are now wrong in a way that matters: a reader who trusts them will either re-implement steps 1-4 or assume `EB_TENANCY_MODE=multi` gives them isolation it does not yet give. So the status lines name the tasks that landed (T1, T2, T3, T4, T13, T22 = §9 steps 1-4) and say plainly that everything from step 5 on — per-user stores, owner-scoped reads, transcript auth, `k8s_tenant.py`, ntfy isolation, triggers, fetched skills, Kafka ACLs — is still design only. The component README gains a Phase 3 configuration table covering only the six variables whose code exists, with two notes that are the point of including it at all: `multi` mode is not yet isolation anyone should present as such (the HTTP reads and `/continue` are still as open as Phase 2's, because steps 5-8 are not in), and adding a user is an operator action because EventBridge has no Kubernetes client. Verified against the code rather than assumed: only `POST /v0/agents` and `POST /v0/groups` call the auth path, so every GET, `/continue` and `/transcript` is open in both modes today. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * fix(eventing): refuse an unresolvable /continue with 503 instead of crashing Found reviewing the previous commit. In `multi` mode `_owner_userkey` returned `None` both when no key was needed (single mode) and when the owning tenant could not be determined. The second `None` flowed into `publish_request` -> `TopicSet.requests(None)`, which raises `ValueError` by design — publishing a resume to a shared topic would run it on another tenant's runner, under that tenant's credential — but nothing caught it, so the request died as a WSGI 500 with a stack trace instead of a status code. Three ways to reach it, none of them abuse: * a user removed from the registry; * a `prompts` row whose `submitter` is NULL — the column is nullable and pre-dates auth, so these rows exist in any store that outlived Phase 2; * a correlation back-filled from the topic by `RequestsMirror`, which records no submitter at all. `_owner_userkey` now returns `(userkey, unresolved_reason)` with exactly one set, so the two `None` cases are distinguishable, and both `/continue` paths check it before writing anything — no touched session row and no orphan prompt row claiming a turn that never ran. The JSON route answers `503` naming what could not be resolved, matching how §2.5 treats a user whose topics do not exist. The HTML form route answers `503` with a plain-text body rather than redirecting: a silent `303` back to the transcript page would look like the turn was accepted and then vanished, which is the Phase 2 §6.1 symptom class this phase keeps trying to avoid. Only reachable with `EB_TENANCY_MODE=multi`, so the default path was never affected. 13 new tests (735 total). Verified they are load-bearing by reintroducing the bug: the four refusal tests fail without the guard and pass with it. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * feat(eventing): Phase 3 T5 — per-user stores and the global ownership index DESIGN_PHASE3.md T5 (§6.1, §2.6), the keystone the rest of step 5+ depends on: T7 (owner-scoped reads), T8 (transcript auth), T15 (triggers) and T21 (deletion) are all blocked on it. `EB_TENANCY_MODE=single` remains the default and reproduces Phase 2. eventbridge/owner_index.py — the one deliberately cross-tenant table `correlationid -> userkey`, UNIQUE on `correlationid` **alone**. That is the part to get right: `(userkey, correlationid)` is the correct primary key inside a tenant's store and exactly the wrong constraint here, because two tenants minting one id produce two distinct tuples, never conflict, and the collision passes through silently — which is the failure the guarantee exists to prevent. It has to be cross-tenant: the lookup that decides which tenant owns a correlation must happen before a per-user store can be chosen. It is also what lets `sessionuuid = uuid5(NAMESPACE, correlationid)` stay UNSALTED (§2.6) — salting it would break every existing session for a collision this constraint removes. `owner_of` returns `(userkey, known)` because a bare `None` conflates "the shared tier owns this" with "never seen", and §6.2's 404 rule needs them distinguished. eventbridge/store_registry.py — per-user stores, LRU-bounded Separate FILES, not one database with a `userkey` column. `store.py` has 28 methods and every one touches tenant data; a `WHERE userkey = ?` remembered 28 times is one that will eventually be forgotten, with a user reading someone else's conversation as the symptom. Separate files make the mistake structurally unavailable — the connection IS the tenant. Two connections per tenant in WAL mode (main + -wal + -shm) bounds this near 150-200 tenants under a 1024 soft RLIMIT_NOFILE, hence `EB_MAX_OPEN_STORES=64`. A store with live SSE subscribers is PINNED: evicting one makes an open page stop updating with no error anywhere. If everything is pinned the cache exceeds its ceiling rather than breaking a viewer — over-budget degrades and is visible on /healthz; a dead stream does not recover. Minter now consults the index (§2.6) One indexed SELECT per mint replaces the startup seeding loop. With N per-user stores the old loop meant N SQLite opens before the socket binds — 100 tenants x 10,000 correlations for a guarantee one SELECT gives directly. The claim happens inside the retry loop, so a candidate lost to a concurrent worker retries instead of both callers believing they own it. `mint_for()` passes `userkey` only to minters that accept it, by signature inspection rather than by catching TypeError around the call — a working `mint()` can raise TypeError from its own body, and swallowing that would silently drop the ownership claim. Routing Responses are filed by the event's OWN signed `ce_userkey`. One with no userkey in multi mode lands in `shared/` and increments `unattributed` (now on /healthz): never guessed into a tenant's store, never dropped. `/continue`, the reads, the group routes and the deadline sweeper all route through the index — the sweeper across every tenant, because a stalled batch belonging to an untouched tenant is exactly what deadlines are for and also the least likely to be in the LRU. This retires the submitter-derived owner lookup from the previous commit, and with it the limitation that it could not tell a GitHub `alice` from a static `alice`. Single-tenant mode keeps its 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 in the UI instead of appearing to vanish. Two bugs found and fixed while building this: * **Eviction could return a closed store.** With every older entry pinned, the LRU took the store it had just opened and handed the caller a closed one — surfacing later as `ProgrammingError: Cannot operate on a closed database` from somewhere that looks unrelated to caching. Fixed with `protect=`, with a regression test. * **`except Exception` hid a signature mismatch.** Adding `userkey=` to `publish_group_event` made every group event stop publishing, and the broad handler around it turned that into a batch that silently never completed. TypeError now re-raises at both call sites. `Store.close()` added (it had none), and it wakes subscribers first so a viewer blocked on `Event.wait()` notices shutdown instead of hanging to its own timeout. Tests: 49 new (785 total), ruff clean. Verified both modes start and that the on-disk layout is upgrade-safe. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * fix(eventing): evict stores by dropping the reference, not by closing them Found reviewing T5. `StoreRegistry` eviction called `Store.close()`, which created a use-after-close race on every multi-call read path: a handler resolves a store, and while it is still making calls a *concurrent request for a different tenant* pushes the cache over `EB_MAX_OPEN_STORES` and closes it underneath. The next call fails with sqlite3.ProgrammingError: Cannot operate on a closed database from a line that looks nothing to do with caching. `get_html` alone makes five calls on one store, so the window is wide rather than theoretical, and reproducing it needs only `max_open=1` and two tenants. Eviction now drops the registry's reference and lets CPython close the connections when the last holder goes away — exactly the lifetime that is safe. File descriptors are reclaimed slightly later than an explicit close; that is the right trade, because the ceiling is a soft budget while a closed connection under an in-flight request is a 500. `_sse_generator` additionally now subscribes BEFORE the replay read. Pinning is what keeps a subscribed store in the cache so later readers get the same object and see the subscriber's notifications, and the old order left the store unpinned between `_store_of()` and `subscribe()`. Reordering is harmless: the Event only ever means "there may be something new", every read is `since_seq=sent`, and an already-set Event costs one extra query. Worth noting what the first fix attempt got wrong, since it is the more interesting mistake: pinning only the SSE path would have closed the case that was easiest to see while leaving every ordinary read exposed. The bug was in the eviction contract, not in one caller. Two regression tests, one asserting an evicted-but-held store stays usable for both reads and subscribe, one pinning the subscribe-before-replay order. 787 passing. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * fix(eventing): teach GroupMirror about tenancy, and stop hiding index failures Three bugs from an adversarial review of T5. All three were confirmed by driving the real code, and all three are multi-tenant-only — the default `single` path is unaffected. **1. Restart-orphaned groups were never settled (`_settle`).** It called `maybe_complete(gid)` with no `userkey`, so it looked in the SHARED store for a group that lives in a tenant's store, found nothing, and returned False. Every group a restart left unfinished stayed unfinished forever, and the failure mode is silence — which §21.6 calls the worst one. The mirror now resolves the owner per group from the ownership index. **2. Replayed member events landed in the shared store.** `on_member_event` reads `ce_userkey` off the event, and any response published *before* Phase 3 has none — so member rows went to `shared/` while the group row sat in the tenant's store, leaving the group's counts permanently wrong and `_decorate_member` reading from a third place. Pre-existing Kafka history is exactly what this mirror exists to replay, so that is the normal upgrade path, not an edge case. The mirror now back-fills a missing `userkey` from the index. It only ever FILLS: an event carrying its own `userkey` keeps it, because that attribute is signed (§2.6) and the index is not authoritative over a signed assertion. Both of these had one root cause — the mirror knew about per-user topics but not about which tenant a replayed group belonged to. **3. A broken index surfaced as "correlation ID space exhausted".** `mint()` caught `Exception` around `claim` to treat a lost race as a retry, so a genuinely broken index (disk full, database locked, schema missing) was retried 2000 times and then reported a message that sends the reader to the word lists rather than the database. Now catches `Collision` specifically; everything else belongs to the caller. Also closes a test-strength gap the review found: deleting the `_has_subscribers` pinning check entirely still passed every `test_store_registry.py` test. The existing tests asserted object identity and counters, not the notification path that is the actual reason for the rule — now that eviction only drops a reference, a held store keeps working, so what actually breaks is that the *writer* gets a new Store whose subscriber map is empty and the viewer is never notified. The new test asserts that end to end; removing the pinning check now fails 4 tests instead of 0. Tests: 8 new (795 total), ruff clean. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * docs(eventing): record T5 as implemented, and what it does not yet give you The status lines said steps 5-12 were design only; T5 (per-user stores and the global ownership index) is now in the tree, so they were wrong in the direction that matters — a reader would either re-implement it or, worse, read "per-user stores" as "isolation". The distinction the docs now draw explicitly: each tenant's sessions, responses and transcripts DO live in their own SQLite files, and a read routes to the owning tenant's store — but nothing yet checks that the *caller* is that owner, because owner-scoped reads (T7) and transcript auth (T8) are not implemented. So `multi` mode's reads, `/continue` and `PUT /transcript` remain as open as Phase 2's. Also adds `EB_MAX_OPEN_STORES` to the component README's Phase 3 table with the descriptor arithmetic that explains the default, and to `test_manifests.py`'s classification list so T22's "every Phase 3 variable is on one side of the Secret-vs-ConfigMap rule" check keeps covering it. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * fix(eventing): address the five must-fixes from the #883 review Every finding reproduced against 16158e5 before fixing, and every fix has a regression test in `tests/test_review_fixes.py` that fails without it (verified by mutation, one fix at a time). They are filed together because they share a cause worth naming: all five were invisible to 192 test functions, because the fixtures assembled the collaborators by hand with the arguments the production call sites fail to supply. **1. `single`-mode regression: the ownership index was never seeded.** The serious one, and against the exact guarantee this phase claims. Dropping Phase 2's `Minter` seeding loop left nothing to populate its replacement, so on the first start after an upgrade `owners.sqlite` is empty while `sessions.sqlite` is full and `exists()` reports live ids as free. With ~1,000 existing correlations a mint has roughly 1-in-25,000 odds of reissuing one, and then `upsert_session` overwrites a session and the new prompt appends to somebody's existing conversation. Phase 2's odds were zero. `OwnerIndex.seed_from()` plus a one-time backfill from every store at startup. §2.6 is right that per-start seeding is the wrong trade; it accidentally argued away the one-time backfill too, and §6.1a now records the distinction. **2. `create_group` did not pass the userkey.** `submit_members` got it and `groups.create()` did not, so in `multi` the group row and the groupid's owner claim landed in the shared store while every member row landed in the tenant's: 0 members forever, `maybe_complete` never fires, the batch never completes and never notifies. A retried POST with an Idempotency-Key read the tenant store `create` never wrote and returned `members: []` — worse than a second batch, because it looks like success. **3. `close_group`/`cancel_group` were unauthenticated cross-tenant mutations.** Neither called `_caller`; both resolved any tenant's group from the global index and acted on it. T7 covers *reads* staying as open as Phase 2's — Phase 2 had no tenants to cross and no cross-tenant mutation anywhere, so this was a capability being introduced, not an openness preserved. Now gated by `_authorize_mutation`, answering `404` for another tenant's group per §6.2's enumeration rule. Single-tenant mode is untouched. **4. `agent` was outside `SIGNED_ATTRS`** while `ce_agent` selects the AgentSpec that supplies `--permission-mode` and the tool allowlists — §5.1's "sandbox". A forged value widened the sandbox and the signature still verified, making a signed deployment worse than an unsigned one because an operator believes it is attested. Added in the same change as `userkey`/`depth`, per §8.3's one-canonicalisation-break rule. **5. `userkey` reached a filesystem path join unvalidated**, from an inbound Kafka header, and `Store.__init__` calls `mkdir(parents=True)` — so `..` created and wrote to a directory inside another tenant's store, or outside the tree. `tenancy.is_valid_userkey()` is the gate, exported from the module that is "the ONLY place an identity becomes a name" so a consumer can check the shape the producer guarantees. An invalid key is treated as a missing one — `shared/` plus `unattributed` — so a forgery is counted and visible rather than acted on. Note it is *truthy*, so the pre-existing `not userkey` check let it past. Also from the review: - **A known-but-unowned correlation 500'd on `/continue`.** `owner=None, unresolved=None` is falsy, so the handler proceeded, wrote a prompt row, then `TopicSet.requests(None)` raised — a traceback with an orphan row already committed, so the conversation showed a turn that was never submitted. Reading and publishing have different requirements, so `_publishable_owner` is now separate from `_owner_userkey`: shared-tier data stays readable, and a turn for it is refused before anything is written. - `claim()` uses `ON CONFLICT DO NOTHING` + a follow-up read, so `Collision` is the only way a conflict presents whether or not two writers race the in-process lock — the bare `INSERT` would have raised `IntegrityError`, which `Minter.mint` deliberately does not catch. - `created_utc` at millisecond precision (matching `ce.now_iso()`) with a `rowid` tiebreaker; at second granularity a 100-member batch had arbitrary order among its rows. - A bad `agent` name is a `400` on the call that made the mistake, not a `202` followed by an async `phase=error` the submitter may never read. - `max_turns: 0` is no longer silently rewritten to the spec's value while the spec path rejects zero. - `EB_TOPIC_PREFIX` validated (it is the one topic-name component `_slug` never sees, and `.`/`_` there reproduce the JMX metric collision) and `EB_MAX_OPEN_STORES` refuses readably instead of raising a bare `ValueError`. - Dropped a flaky assertion: `"gh" not in` 26 characters of base32 fails ~2.4% of the time by construction, and the `"mrsabath" not in` line above it is the real property. - `_canonicalise`'s docstring no longer claims static identities are byte-exact when the `@` branch catches address-shaped ones first. Design doc, where the code had disproved it (§9's "the code is the authority" rule): §6.1's layout corrected to put single-tenant stores in the bridge root; "Closing is safe" replaced with what commit 49aed05 established, including that the first fix attempt only pinned the SSE path and left the class open; new §6.1a documents `owner_index.py`; §2.6's "one new extension attribute" corrected to four, with which three are signed and why; §3.1 records the single-mode `events`/`dead` names. The status header now warns that `multi` does not work end to end until T6 — no response is consumed, which is more useful than "not yet isolated". Tests: 45 new (840 total), ruff clean. Still outstanding from the review and deliberately not in this commit: the T22 subset check, `RequestsMirror`/`NtfyPublisher` `stores=`, the handler-to-topic integration test, the `forget()` tombstone decision, and the four documents. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * fix(eventing): the four remaining #883 suggestions, and tombstone deletion Round two of the review, covering what was deferred from 6ebdd80. **T22 now checks the direction that enforces the rule.** `CONFIGMAP_VARS ⊆ found` catches a stale entry and nothing else; the direction that catches a *new* variable classified as neither is `found ⊆ CONFIGMAP_VARS ∪ SECRET_PATH_VARS`, which is the case the test exists for. Scoped to Phase 3's variables — the 36 that pre-date §8.3 are listed explicitly, with `test_the_baseline_list_is_still_accurate` keeping that list honest rather than letting it silently widen the exemption. Verified by deliberate failure, as asked: injecting an unclassified `e("EB_UNCLASSIFIED_X")` read into `config.py` turns the test red with the variable named. Also replaced the hardcoded ten-space indentation in the inlined-secret check with `\s*`, matching the precedent elsewhere in the same file — the old pattern would miss the same variable inlined in an initContainer, a sidecar or a deeper patch. **`RequestsMirror` and `NtfyPublisher` now take `stores=`.** Both knew the per-tenant topic names and still wrote to the shared store, which does look like the sweep in 77807b6 not reaching them. The mirror back-filled every prompt into `shared/`, so a correlation whose request the bridge did not originate showed a BLANK PROMPT on its owner's page — the exact gap the mirror exists to close. ntfy's `_last_prompt` and `_last_assistant_text` read only the shared store, so a tenant's notification arrived without the prompt or the reply, which §4.1 says is the content that lets a phone that cannot reach EventBridge still see the result. Its group-existence check is routed too. **A handler→topic integration test**, which the review argued was a better investment than more unit tests on the pieces. It is: faking at `KafkaProducer` rather than at `Producer` means the real handler, the real `Producer` and the real `TopicSet` all run, and the assertion is *which topic the bytes went to*. Confirmed by reverting each fix that it catches both defects it was written for — the `create_group` missing `userkey` and the `/continue` 500 on an unowned correlation. That test immediately earned itself twice over: the mutation run surfaced that `_publish_started`/`maybe_complete` swallow `ValueError` from `TopicSet` into a log line, so a caller forgetting a `userkey` produced a group that silently never announced itself — the same shape as the `TypeError` already guarded there. Both guards now cover both. **Deletion tombstones rather than freeing the id.** This is the one I said needed thought rather than a quick answer, and the answer is that reuse is unsafe. Verified: `forget()` used to `DELETE` the row, `exists()` is the `Minter`'s only uniqueness check, and `sessionuuid = uuid5(NAMESPACE, correlationid)` is unsalted by design — so a reissued `correlationid` derives *the same session uuid*, and a `claude` transcript left on a runner's volume becomes resumable by the new correlation. That is one user's conversation continuing inside somebody else's agent. So `forget()`/`forget_tenant()` keep the row, set `deleted_utc` and clear `userkey` — the id is reserved forever, and the tombstone discloses nothing about whose it was. The alternative is guaranteeing every transcript keyed on that uuid is purged everywhere, including volumes this process does not own, which is not a guarantee EventBridge can make. Cost is one short row per deleted correlation. Migrated with an `ALTER TABLE`, the same idiom `store.py` uses for `prompts.submitter`, and a test covers an index created before the column existed. The reviewer was also right that the docstring contradicted the code: it claimed "freeing the id for reuse is deliberate" about a `DELETE` nobody had reasoned about. Design doc: §6.1a records the tombstone decision and why, since the index is what enforces it. Tests: 855 passing (+13), ruff clean. Still outstanding: the four phase documents (`IMPLEMENTATION_REPORT3.md`, `README_PHASE3.md`, and the two owed Phase 2 documents). Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * docs(eventing): IMPLEMENTATION_REPORT3.md — what Phase 3 built, measured Asked for in the #883 review, and the argument for writing it now rather than later was the most persuasive one there: the findings are attributable to specific commits today and will not be in a month. 361 lines, following `IMPLEMENTATION_REPORT1.md`'s contract — measured figures rather than estimates, failures with root causes, and an explicit account of what is NOT verified. Phase 0 and Phase 1 each ship three documents; Phase 2 shipped none, and the gap shows in that there is nowhere to put the §6.1 split-consumer trap except inside a design document. This starts closing that for Phase 3. **Measured rather than asserted.** §6.1 estimates the per-tenant descriptor cost and derives a ceiling; measured it is 6.0 descriptors per open store (10 tenants, 60 fds), which puts ~680 tenants under this host's 4096 soft limit and ~170 under the 1024 the design assumed — so squarely inside its "150-200" once the limit is held constant. The ownership index costs 27.4 µs per `claim`, 1.7 µs per `exists`, and 270 ms to seed 10,000 ids. That last figure is the one that matters: it settles §2.6's objection to seeding, because the *one-time* backfill it appeared to rule out costs 270 ms once, while the *per-start* seeding it correctly rejects is what the index exists to avoid. **Seven deltas from the design, recorded as decisions rather than drift** — the single-tenant store location, eviction by reference-drop, tombstoned deletion, the single-mode `events`/`dead` names, `agent` joining `SIGNED_ATTRS`, the exported userkey validator, and prefix validation. **The findings, ordered by what they cost to find.** Three are worth the document on their own: - The `single`-mode regression shipped behind a headline claiming it could not happen, with the reproduction (50 of 50 live ids reported free), the 1-in-25,000 arithmetic over a verified 50x50x10,000 id space, and the three reasons it is instructive: it was in the default configuration, the suite could not see it because no fixture simulated an upgrade, and §2.6's wording contributed. - The eviction contract, where the lesson is the *first* fix: pinning only the SSE path addressed the case that was easy to picture and left the class open. - `except Exception` converting three separate programming errors into log lines, on three separate occasions, which is why it is filed as one finding with the pattern extracted. Also records the suite's structural blind spot as a finding in its own right — a fake placed at the boundary under test cannot test that boundary — since that is what let two defects survive 200-odd new test functions. **§5 is the longest section, deliberately.** `multi` does not work end to end until T6 (no response is consumed); no cluster, no two-tenant deployment, no Kafka; reads not owner-scoped; Tier A is not isolation; the per-tenant throughput ceiling §10 asks for is still unmeasured; the `claude --help` flag assertion does not run in CI; `seed_from` is capped at 10,000 per store; and `is_valid_userkey` is shape validation, not authorisation. Every figure in the report was measured on this branch, every cited commit verified to be on it, and every cited design section verified to exist — §21.6 turned out to be `DESIGN_PHASE1.md`'s and is now qualified as such. Two numbers in my own PR description were wrong and are corrected here: the test delta is +232 unique functions (525 to 757), not the 139 or 217 claimed earlier. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * fix(eventing): two orphan-write paths, and three design/code gaps (#883 round 2) Both must-fixes reproduced against c25a0c7 before fixing, and both are my own round-1 fix introducing a new bug class: `6ebdd80` added validation of the request's `agent` name at the HTTP boundary — right place, right reason — and placed it AFTER the writes. **create_group** had already committed the group row and published `group.started` when the 400 returned, and because the row is keyed on the Idempotency-Key the state was permanent: every retry read back `created: false, members: []`, which looks like success. That is verbatim the failure the `userkey=` comment added in the same round describes, reached by a route that fix did not cover. 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: []} **start_agent** had written the session row, the prompt row and claimed the correlation id — a transcript showing a turn that was never submitted, plus an id nothing ever un-claims, since tombstoning is for deletion rather than abandonment. Both resolutions now happen before the first write. `_agent_for` needs only `caller` and `body`, so there was never anything to gain by deferring it. After the fix both paths publish nothing, write nothing, and the group retry launches the batch properly. The reviewer's observation about the tests is the more useful half: the assertion that would have caught both — `get_prompts(corr) == []`, *a refusal writes nothing* — already existed 25 lines earlier in the same file, on a different route. The new tests assert it on both paths, with the Idempotency-Key retry for the group case, and were verified by reverting each ordering (7 failures). ### Three design/code gaps §3 of the report had missed **The backfill was per-start, not one-time.** §6.1a says "seeded once" and the report said "once, on the first start after an upgrade"; the loop re-opened every tenant store and re-read `all_correlations` on every boot — exactly the N-store startup seeding §2.6 rejects, reintroduced by the fix for the missing seed. `INSERT OR IGNORE` kept it correct; what repeated was the cost §2.6's argument is entirely about. A `seed_state(scope, done_utc)` table now records each scope, and `needs_seeding` is checked before the store is opened. Measured at 100 tenants x 200 correlations: before: start 1 702 ms / 101 opens start 2 210 ms / 101 opens after: start 1 702 ms / 101 opens start 2 0.8 ms / 1 open Sound because nothing can add an unindexed id to a seeded store: every mint claims as it mints. **`AgentSpec.limits` was parsed and enforced nowhere, silently.** `Skill` states plainly that it carries `sha256` for T19; `Limits` made no such disclaimer, so `timeout_s = 900` loaded clean and did nothing. Now says so, with which field belongs to which later task. Its three conversions also escaped the module's own `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 disagreed with §2.6 and the code.** It still said the signed set "changes twice" and named two attributes, while §2.6, `signing.py` and `ce.py` all said three — and `signing.py` cited §8.3 as the authority for a rule §8.3 stated over the wrong count. Two smaller ones from the same pass: a tombstoned correlation could be re-claimed by the NULL owner (`row[0] == userkey` reads as "already mine" when both are NULL), contradicting what `forget` guarantees — unreachable from `Minter.mint`, which checks `exists()` first, but reachable as soon as 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; a typo there meant an asynchronous `phase=error` on every request from that user rather than a startup refusal. Verified: the registry now refuses with the bad value named. ### Documentation - The component README's Phase 3 block no longer orphans the "Two of these are load-bearing" sentence from its payoff 25 lines later — the block moved below it under its own `#### Phase 3` heading, and the note count is now right (three, not two). Added the T6 "does not work end to end" warning here too. - `IMPLEMENTATION_REPORT3.md` §1 now counts from a named point (`3278d75`) instead of "currently", since a report cannot be right about the diff of the commit containing it; both figures are given. New §4.7 and §4.8 record this round's findings, including that fixing one orphan-write instance did not teach me to look for the class — the same lesson §4.2 records about the eviction contract, learned a second time. Tests: 860 passing (+5), ruff clean, both multi-mode startup paths smoke-tested. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * test(eventing): Phase 3 — regression tests for the three untested fixes (#883) From the #883 approval: reverting `agentspec`'s `try/except`, `claim`'s tombstone check or `registry`'s `validate_name` left all 858 tests passing, so three of the six fixes in `258da7a` were invisible to the suite. The standard this PR has applied since `6ebdd80` is "every fix has a regression test, verified by reverting its fix", and it had been applied to half that commit — and, as the review put it, the three without one are the three whose absence is invisible. Fourteen tests, each verified by reverting its fix independently: | Fix | Failures when reverted | |---|---| | `[limits]` conversions wrapped in `SpecError` | 4 | | tombstone never re-claimable | 1 | | `registry.parse` validates `agent` | 5 | The tombstone test covers the case that was actually wrong — `claim(corr, None)` against a tombstoned row, where `row[0] == userkey` is `None == None` and read as "already mine" — and also asserts a *tenant* re-claim now fails with "tombstoned" rather than "already owned by (single-tenant)", so the message points at the real reason. The registry tests cover the shapes `validate_name` rejects and, separately, that a valid name loads and an absent one stays empty so `ER_AGENT_NAME` still applies. A gate that rejects what it should allow is the other way to get this wrong. **Also closes the range-check gap the review flagged as on-the-record.** `timeout_s = -5` and `max_events = 0` loaded clean: timeout_s = -5.0 | max_events = 0 Defensible while `Limits` is documented inert, but a negative deadline is nonsensical input whether or not anything reads it yet, the module's own standard is that a policy which cannot be understood fails to load, and the validation was already right there. Rejecting now also means whoever wires up the `Popen` wrapper inherits a value they can trust instead of re-validating. `0` keeps its documented meaning for `timeout_s` and `max_output_bytes` ("no deadline" / "unbounded"), with a test pinning that the range check does not reject the sentinels; `max_events = 0` is refused because a run that may emit no events cannot report its own result. Tests: 875 passing (+14), ruff clean. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * test(eventing): extend the §8.3 rule to #884's seed paths, and cover kind-signed Follow-up to rebasing onto `main` now that #884 and #890 have merged. Both predicted conflicts were in `agentdocs/README.md` and resolved as planned — `main`'s Phase 2 row (which gained §8 from #890) plus this branch's Phase 3 row. `test_manifests.py` auto-merged, keeping #884's `allowed_secrets` fix beside this branch's Phase 3 classification. That left two things the merge did not resolve by itself. **Extending `SECRET_PATH_VARS` to #884's seed paths needed the rule split, not widened.** #884's review predicted this tuple would be extended to `ER_/EB_SIGNING_KEY_PATH` and checked that doing so stays green. It does for the ConfigMap half and would NOT for the literal-value half, because those two variables hold a **mount point, not key material** — and #884 deliberately sets them as container `env` entries so the path sits beside the volume that supplies the seed, which is what `configmap.yaml` asks for. So the tuple is now two: - `SECRET_VALUE_VARS` — the three whose *value* is key material. No literal value anywhere, which is the original assertion. - `SEED_PATH_VARS` — #884's two. A literal value is correct; a ConfigMap key is not. Both still obey "never from `config.toml`", so that check covers all five now where it covered three. Treating a path naming a secret as if it were the secret would have forced #884 to either revert its own correct fix or carve itself out of the rule — the distinction is the thing worth encoding. **`kind-signed` was not in `OVERLAYS`.** #884 added the overlay and every assertion in this file silently skipped it — including the seed-path rule the PR exists to respect, the `replicas` invariants, and the no-credential checks. Added, which is also what makes the new assertion meaningful: with `kind-signed` absent, injecting a seed path back into `eventing-config` passed; with it present, that regression fails as it should. Verified both ways. Tests: 899 passing (875 here + #884's 24 picked up in the rebase), ruff clean, all four overlays build. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> --------- Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
…mechanism (#892) * docs(eventing): Phase 2 — make §8's count checkable, and correct one mechanism Follow-up to the #890 review, which landed before these were addressed. Three findings, all about §8 rather than the code it describes. **The count did not reconcile, in §8 of all places.** "Thirteen" appears six times in this file and twice in `agentdocs/README.md`, and §8.1–§8.3 enumerated **eleven**. The number was reachable only by knowing that two bullets each carried two findings in one paragraph — §8.2's capability-URL entry covered both `GET /v0/groups` and `GET /v0/groups/{id}/status`, and §8.3's SPIRE entry covered both the algorithm and the keyset file format — and nothing in the text marked them as two. Both are now split, so the list reads 13, and §8 carries the tally explicitly: five in §8.1 (four rows plus the fifth, promised nowhere), four in §8.2, four in §8.3. This mattered more here than it would anywhere else in the repository, which is the reviewer's point and the right one: §8 exists to argue that unchecked claims survive review, so a count that does not survive counting is the same failure in miniature — and it is the first thing a sceptical reader tests. **§8.1's group-event row named the wrong field and the wrong causal step.** It said `kafka_in.py` "keeps `final` and `groupid`, so the rewritten event still reaches `on_group_event`". Verified against the code: a group event carries **no `final` at all** (`publish_group_event` never sets it, `on_group_event` never reads it), and routing is on `ce.is_group_event(evt)` — computed on the *original* event, before and regardless of the rewrite. So the rewrite is not why it reaches the handler; `type` and `groupid` surviving is why the batch then ends. Corrected to the reviewer's wording. Worth getting exactly right because §8.5.1 is "**Which code path, by name?**" and this row is the worked example a reader copies the style from. **One finding had no issue, in the section that says to file the issue.** The ntfy group-suppression finding was the only one of the thirteen with no link, and #887 does not cover it — that one is `PUT /transcript` and listable ids. Filed as #891 and linked, with a pointer to §8.5.4 as the general rule it instantiates. While checking that, §8.3's items turned out to have no links either — correctly, because they are documentation drift rather than code bugs, so the correction *is* the fix. §8.3 now says so, since §8.7 tells readers to file issues and an unlinked finding otherwise reads as an oversight. Verified every `§N.N` reference in the file still resolves to a heading that exists; the only unresolved one is the pre-existing, explicitly qualified "Phase 1 §16". 575 passing, ruff clean. No code changes. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * docs(eventing): Phase 2 — warn that `op: add` on `env` replaces the list The remaining nit from the #884 approval, which merged before it was addressed. My comment on both patch blocks warned about the failure that is **loud** — an `env/-` append failing with "path does not exist" when no list exists — and not about the one that is **silent**: `op: add` on `…/containers/0/env` *replaces* the list when one already exists. Safe in `kind-signed`, which builds on `../kind` whose containers have no `env`. Not safe in the overlay the PR plans to copy this patch into. Reproduced by building a throwaway `demo-signed` that changes nothing but the base: demo-signed eventrunner env = ['ER_SIGNING_KEY_PATH'] demo's own = ['ER_MOCK_CLAUDE', 'HOME', 'CLAUDE_CONFIG_DIR'] -> all three LOST, silently `kustomize build` succeeds and nothing warns. Losing `ER_MOCK_CLAUDE=false` means a `demo-signed` overlay would run the demo in **mock mode** — which is exactly what `test_the_test_overlay_mounts_no_credential_so_mock_mode_is_automatic` exists to keep honest at the other end, so the failure would be invisible from both directions. The comment now says that, and names the fix for whoever writes that overlay: `op: add` on `env/-` per entry, against a base that already has the key. Reviewer's words, and the right call to spend half a sentence on — this file is the starting point for three more (preflight, e2e, demo flow) by the PR's own plan. 575 passing, `kind-signed` still builds, no behaviour change. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> * docs(eventing): Phase 2 — correct the kustomize env hazard to real mode The warning said that a `demo-signed` overlay dropping `../demo`'s `env` list would "run the demo in MOCK MODE". It would not. `op: add` on `/spec/template/spec/containers/0/env` replaces the `env` list, but `envFrom` is a sibling field a JSON patch on `env` cannot reach. So `anthropic-credentials` (`optional: false`) still supplies `ANTHROPIC_AUTH_TOKEN`, `ER_MOCK_CLAUDE` is unset, and `eventrunner/config.py:127` takes the auto-detect branch. Driving the real `config.load()` with the surviving environment: mock_claude = False mock_reason = 'auto: API credential present' Real mode, the opposite of the warning. What is actually lost is `HOME`/`CLAUDE_CONFIG_DIR`: claude writes session transcripts under `$HOME/.claude`, so they move to the container's ephemeral layer and a restart loses every resumable session on the pod — the §16 Gap B failure the demo patch's own comment warns about. The copy of the warning on the **eventbridge** patch also cited three `env` entries that `../demo` only ever adds to **eventrunner**; `../demo` patches eventbridge's `volumes/0` and nothing else. That patch now carries the general rule and points at the eventrunner patch for the worked example. Also from the same review: the §8 scope line localises the thirteen findings to §8.1–§8.3 now that the tally is explicit, since §8.4 is commentary and enumerates none; and a stray double blank line left by removing the old §8.3 heading. `kustomize build` on the overlay still succeeds. No manifest field changed — comments, and three lines of DESIGN_PHASE2 prose. Assisted-By: Claude Code Signed-off-by: Mariusz Sabath <mrsabath@gmail.com> --------- Signed-off-by: Mariusz Sabath <mrsabath@gmail.com>
Phase / scope
DESIGN_PHASE2.md)DESIGN_PHASE3.mdindex row, which #883 rewrites.DESIGN_PHASE2.mditself made about the identity and signing controls. The procedure derived from them is phase-agnostic and says so.Updated per @aslom's review (
15b81da):CLAIMS.mdis merged intoDESIGN_PHASE2.md§8 and the standalone file is gone — see "Addressing the review"below.
Lessons from the eventing docs review (rossoctl/rossoctl#2609, now approved), written
back into the design record.
What happened
Writing the user-facing pages meant restating every claim in
DESIGN_PHASE2.mdfor areader who cannot see the code. Six review rounds then checked each restated claim by
running it, against
eventing/atd9677ddd. Thirteen did not hold.Not because the design record is vague about mechanism — §4.4 is unusually precise. Each
claim was written from the change that introduced the control, and stayed true for the
configuration that change was exercised in. The half-configured cases, and the cases a
later change moved, are where all thirteen were found. Every one coexisted with a
passing test suite.
Addressing the review
@aslom asked for
CLAIMS.mdto be merged into a design document, "as the claims arepart of design and they should reflect the phase of the design they are related to".
Done in
15b81da:CLAIMS.mddeleted; its content is now §8.5–§8.8 ofDESIGN_PHASE2.md, directlyafter the thirteen findings it was derived from. The content moved rather than being
rewritten (133 lines out, 135 in); the five questions became §8.5.1–§8.5.5 and the
three references that assumed a separate file are re-pointed.
§8 now opens by stating which half is phase-specific, which is the substance of
the request:
agentdocs/README.md: standalone row removed, the Phase 2 row covers both halves, andthe reading-order entry is "Writing a claim about a control, in any phase?" with
"The findings are Phase 2's; the questions are not" stated outright.
That also settles question 1 from the original description, which asked whether
CLAIMS.mdwas the right home — the answer is no, and the phase-aligned placement isbetter than repo root for the reason §8.5 now gives: a procedure filed away from the
findings that motivated it is a procedure nobody reads.
The changes
DESIGN_PHASE2.mdagentdocs/README.md§8 is additive — §§2.6, 3, 4.3 and 4.4 are left as written, following this repo's
existing convention ("where Phase 1 supersedes Phase 0 it says so explicitly rather than
editing in place"). A correction that deletes the intent loses the more useful half.
The four §4.4 promises that fail
group.completed"finalandgroupidINSERT OR REPLACElets a later unsigned frame overwrite the verified terminal rowEB_SIGNING_KID"And a fifth, promised nowhere and worse than any of them:
EB_REQUIRE_RESPONSE_SIGNATURE=truewith no keyset refuses nothing.requireisnever consulted without a keyset, so a forged terminal is stored as an answer and
startup says
response verification OFF. The request side with no key at all refusesevery request. The two sides fail in opposite directions and only one is loud (#888).
Relationship to the other drafts in flight
Stated by phase, since all three touch
eventing/:kafka_in.pyonly re-points the store for tenancy. §8 stays accurate on top of #883, though line numbers move. We both editagentdocs/README.md; whichever merges second resolves one table row and one reading-order entry.Branched from
mainrather than #883 so this is reviewable on its own.Remaining question
Should §8 name the fix, or only the finding? Right now it says what the code does and
links the issue, with proposed fixes left in the issues on the grounds that a design
record should not pre-empt the fix discussion. If you would rather §8 carry the intended
resolution so the record is self-contained, that is a small change.
(The original description also asked whether
CLAIMS.mdbelonged at repo root — answeredabove and implemented.)
Not included
No code changes. Issues #885–#889 carry those, and the fixes should land as their own PRs
with tests rather than riding along with a docs change.
🤖 Generated with Claude Code