From 0b7d8c1503e35afc57548318eec288fe7b8aa168 Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Mon, 5 Oct 2026 18:48:23 -0400 Subject: [PATCH 1/3] =?UTF-8?q?docs(eventing):=20Phase=202=20=E2=80=94=20m?= =?UTF-8?q?ake=20=C2=A78's=20count=20checkable,=20and=20correct=20one=20me?= =?UTF-8?q?chanism?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Signed-off-by: Mariusz Sabath --- eventing/agentdocs/DESIGN_PHASE2.md | 41 ++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/eventing/agentdocs/DESIGN_PHASE2.md b/eventing/agentdocs/DESIGN_PHASE2.md index 6e49703d..a5ae8a07 100644 --- a/eventing/agentdocs/DESIGN_PHASE2.md +++ b/eventing/agentdocs/DESIGN_PHASE2.md @@ -458,6 +458,13 @@ document for a reader who cannot see the code. Six review rounds then checked ea restated claim **by running it**, against `eventing/` at `d9677ddd`. Thirteen did not hold. +The tally, because a count that cannot be checked is the same failure this section is +about: **five in §8.1** (four table rows plus the fifth, promised nowhere), **four in +§8.2**, **four in §8.3**. An earlier revision stated thirteen while enumerating eleven, +because two bullets each carried two findings in one paragraph — §8.2's capability-URL +entry covered the group list and the member list, and §8.3's SPIRE entry covered the +algorithm and the file format. They are split now, so the list reads 13. + They are recorded here rather than edited into §3 and §4.4 in place, so the reasoning above stays readable as the record of what was intended, and this section says what the code does. That convention is this repository's own: where a later phase supersedes @@ -469,7 +476,7 @@ deletes the original intent loses the more useful half. | §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) | +| 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) | | Step 2: "this proves *who finished a run*" | Not once stored. `insert_response` uses `INSERT OR REPLACE` on `(correlationid, sequence)`, so a later **unsigned** frame reusing a sequence number replaces the verified terminal row. | [#885](https://github.com/rossoctl/examples/issues/885) | | "EventBridge refuses to start with a keyset but no `EB_SIGNING_KID`" | True — but nothing checks that the kid is *in* the keyset. EventBridge starts, then flags its own group events. | [#888](https://github.com/rossoctl/examples/issues/888) | @@ -486,10 +493,12 @@ opposite directions, and only one of them is loud.** ### 8.2 §3's "deliberately left open" is wider than stated -- **§3.1's capability-URL argument rests on ids being unguessable.** `GET /v0/groups` - returns the 100 most recent groups without sign-in, and `GET /v0/groups/{id}/status` - lists every member's correlation id. For any conversation in a group, the capability - is published. ([#887](https://github.com/rossoctl/examples/issues/887)) +- **§3.1's capability-URL argument rests on ids being unguessable, and the group list + publishes them.** `GET /v0/groups` returns the 100 most recent groups without sign-in. + ([#887](https://github.com/rossoctl/examples/issues/887)) +- **The member list publishes them too.** `GET /v0/groups/{id}/status` lists every + member's correlation id, so for any conversation in a group the capability is published + rather than merely guessable. ([#887](https://github.com/rossoctl/examples/issues/887)) - **§3.2 understates what `PUT /transcript` allows.** It is the checkpoint EventRunner **resumes from** on a cold pod, so an unauthenticated write changes what the agent continues with. Request signing does not cover it. @@ -499,16 +508,28 @@ opposite directions, and only one of them is loud.** 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. + ([#891](https://github.com/rossoctl/examples/issues/891)) — §8.5.4 is the general rule + this is an instance of, and the fix is to decide on group membership from the store + rather than on the groupid the frame supplies. + +### 8.3 Four things about the keys + +**No issue links in this subsection, deliberately.** §8.1 and §8.2 record code bugs and +every item carries the issue filed for it. These four are **documentation drift** — claims +this document made that the code never matched, or stopped matching — so the correction +*is* the fix and there is nothing to track. §8.7 says to file the issue; it is worth +saying out loud where that does not apply, since an unlinked finding otherwise reads as +an oversight. -### 8.3 Three things about the keys - **§2.6 records `submitter`/`submitteriss` as unsigned.** They joined `SIGNED_ATTRS` in §4.2, so the signature covers them. The limit that remains is a different one and worth stating as such: a signature proves EventBridge *asserted* the name. -- **§4.3's "the verification code does not change" under SPIRE does not hold.** The - verifier accepts EdDSA only, and SPIRE issues EC or RSA keys. The keyset is also a - flat JSON map of kid to key, and `keyset.load` rejects a JWKS document outright. The - kid lookup survives; the algorithm and the file format do not. +- **§4.3's "the verification code does not change" under SPIRE does not hold, on + algorithm.** The verifier accepts EdDSA only, and SPIRE issues EC or RSA keys. +- **Nor on file format.** The keyset is a flat JSON map of kid to key, and `keyset.load` + rejects a JWKS document outright. The kid lookup survives; the algorithm and the file + format do not. - **A flat keyset makes the asymmetric keys attribution, not restriction.** §4.4 step 5 says a flat keyset "is not enough" for group events, and pins them. The same reasoning applies to member answers and is not drawn: EventBridge's own key is in From adcff2a825ebbb28ff52ca5f0690551a92c527aa Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Mon, 5 Oct 2026 18:53:50 -0400 Subject: [PATCH 2/3] =?UTF-8?q?docs(eventing):=20Phase=202=20=E2=80=94=20w?= =?UTF-8?q?arn=20that=20`op:=20add`=20on=20`env`=20replaces=20the=20list?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Signed-off-by: Mariusz Sabath --- .../k8s/overlays/kind-signed/kustomization.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/eventing/k8s/overlays/kind-signed/kustomization.yaml b/eventing/k8s/overlays/kind-signed/kustomization.yaml index e0a805d9..dd100d70 100644 --- a/eventing/k8s/overlays/kind-signed/kustomization.yaml +++ b/eventing/k8s/overlays/kind-signed/kustomization.yaml @@ -71,6 +71,14 @@ patches: # `env` does not exist on the base container (only `envFrom`), so this adds the # whole list rather than appending to it. A trailing-slash `env/-` op would fail # with "path does not exist" at render time. + # + # **The silent hazard is the other direction:** `op: add` on `…/env` REPLACES the + # list when one already exists. Safe here because this overlay builds on + # `../kind`, whose containers have no `env` — but `../demo` has three entries, so + # a `demo-signed` overlay built by changing only the base would silently drop + # `ER_MOCK_CLAUDE=false`, `HOME` and `CLAUDE_CONFIG_DIR` and run the demo in MOCK + # MODE. `kustomize build` succeeds and nothing warns. Whoever writes that overlay + # wants `op: add` on `env/-` per entry, against a base that already has the key. - op: add path: /spec/template/spec/containers/0/env value: @@ -83,6 +91,14 @@ patches: # `env` does not exist on the base container (only `envFrom`), so this adds the # whole list rather than appending to it. A trailing-slash `env/-` op would fail # with "path does not exist" at render time. + # + # **The silent hazard is the other direction:** `op: add` on `…/env` REPLACES the + # list when one already exists. Safe here because this overlay builds on + # `../kind`, whose containers have no `env` — but `../demo` has three entries, so + # a `demo-signed` overlay built by changing only the base would silently drop + # `ER_MOCK_CLAUDE=false`, `HOME` and `CLAUDE_CONFIG_DIR` and run the demo in MOCK + # MODE. `kustomize build` succeeds and nothing warns. Whoever writes that overlay + # wants `op: add` on `env/-` per entry, against a base that already has the key. - op: add path: /spec/template/spec/containers/0/env value: From 3e1dfa1ce30b3e47d97a23d4630f2555d744e66f Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Tue, 6 Oct 2026 09:50:17 -0400 Subject: [PATCH 3/3] =?UTF-8?q?docs(eventing):=20Phase=202=20=E2=80=94=20c?= =?UTF-8?q?orrect=20the=20kustomize=20env=20hazard=20to=20real=20mode?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- eventing/agentdocs/DESIGN_PHASE2.md | 6 +++--- .../overlays/kind-signed/kustomization.yaml | 21 ++++++++++++------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/eventing/agentdocs/DESIGN_PHASE2.md b/eventing/agentdocs/DESIGN_PHASE2.md index a5ae8a07..ca5354c3 100644 --- a/eventing/agentdocs/DESIGN_PHASE2.md +++ b/eventing/agentdocs/DESIGN_PHASE2.md @@ -447,8 +447,9 @@ knowing before a demo. ## 8. What the docs review found (added after rossoctl/rossoctl#2609) -**Scope.** §8.1–§8.4 are about **Phase 2** specifically: thirteen claims *this +**Scope.** §8.1–§8.3 are about **Phase 2** specifically: thirteen claims *this document* made about the identity and signing controls, which do not hold in the code. +§8.4 is commentary on why they all failed the same way, and enumerates none of them. §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. @@ -521,7 +522,6 @@ this document made that the code never matched, or stopped matching — so the c saying out loud where that does not apply, since an unlinked finding otherwise reads as an oversight. - - **§2.6 records `submitter`/`submitteriss` as unsigned.** They joined `SIGNED_ATTRS` in §4.2, so the signature covers them. The limit that remains is a different one and worth stating as such: a signature proves EventBridge *asserted* the name. @@ -550,7 +550,7 @@ document is written. ### 8.5 Checking a control claim before it ships -§8.1–§8.4 are what thirteen unchecked claims cost. What follows is the procedure that +§8.1–§8.3 are what thirteen unchecked claims cost. What follows is the procedure that came out of them: five questions to ask of any sentence that says a security control does something, how to check it cheaply, and what a correction should preserve. diff --git a/eventing/k8s/overlays/kind-signed/kustomization.yaml b/eventing/k8s/overlays/kind-signed/kustomization.yaml index dd100d70..a380188a 100644 --- a/eventing/k8s/overlays/kind-signed/kustomization.yaml +++ b/eventing/k8s/overlays/kind-signed/kustomization.yaml @@ -74,11 +74,11 @@ patches: # # **The silent hazard is the other direction:** `op: add` on `…/env` REPLACES the # list when one already exists. Safe here because this overlay builds on - # `../kind`, whose containers have no `env` — but `../demo` has three entries, so - # a `demo-signed` overlay built by changing only the base would silently drop - # `ER_MOCK_CLAUDE=false`, `HOME` and `CLAUDE_CONFIG_DIR` and run the demo in MOCK - # MODE. `kustomize build` succeeds and nothing warns. Whoever writes that overlay - # wants `op: add` on `env/-` per entry, against a base that already has the key. + # `../kind`, whose containers have no `env`. That is a property of this base, not + # a general guarantee — see the eventrunner patch below, where `../demo` does add + # an `env` list that a `demo-signed` overlay would silently replace. `kustomize + # build` succeeds and nothing warns either way. Whoever writes that overlay wants + # `op: add` on `env/-` per entry, against a base that already has the key. - op: add path: /spec/template/spec/containers/0/env value: @@ -96,9 +96,14 @@ patches: # list when one already exists. Safe here because this overlay builds on # `../kind`, whose containers have no `env` — but `../demo` has three entries, so # a `demo-signed` overlay built by changing only the base would silently drop - # `ER_MOCK_CLAUDE=false`, `HOME` and `CLAUDE_CONFIG_DIR` and run the demo in MOCK - # MODE. `kustomize build` succeeds and nothing warns. Whoever writes that overlay - # wants `op: add` on `env/-` per entry, against a base that already has the key. + # `ER_MOCK_CLAUDE=false`, `HOME` and `CLAUDE_CONFIG_DIR`. Mock mode is *not* the + # failure: `envFrom` is a sibling field and survives, so the credential is still + # present and config.py auto-detects **real** mode ("auto: API credential + # present"). What is lost is `HOME`/`CLAUDE_CONFIG_DIR`, so claude writes session + # transcripts to the container's ephemeral layer instead of the mounted volume and + # a restart loses every resumable session on the pod — the §16 Gap B failure. + # `kustomize build` succeeds and nothing warns. Whoever writes that overlay wants + # `op: add` on `env/-` per entry, against a base that already has the key. - op: add path: /spec/template/spec/containers/0/env value: